From 358479f075a5d0099fa4fcc72945bfff8f26139b Mon Sep 17 00:00:00 2001 From: t Date: Sat, 11 Jul 2026 19:40:10 -0500 Subject: [PATCH 1/4] fix(git-remote): drive multi-round fetch as a v0 stateless-RPC client loop (#117) git-remote-gitlawb advertised `connect`, so git spoke the stateful native protocol, but handle_connect collapsed the exchange into one GET plus one POST. A multi-round fetch (more than ~32 overlapping commits) deadlocked: git sent a flush-terminated have-batch and blocked for an ACK/NAK the helper never sent, while the helper blocked reading for a `done` git never sent. Turn Phase 2 into a per-round stateless-RPC client loop. read_upload_pack_round returns one round at a time (at a flush, done, or EOF) instead of buffering past flushes waiting for `done`; negotiate_upload_pack captures the wants once and POSTs a self-contained request per flush-terminated batch: wants, one flush, every have accumulated so far, and exactly one terminator. Each ACK/NAK is streamed back to git so it advances, until the done round returns the pack. The node's `git upload-pack --stateless-rpc` keeps no state between POSTs, so wants and all prior haves are re-sent every round and no intermediate flush survives into a body (which would truncate the negotiation server-side). Signing is preserved per round: every POST carries the Phase-1 decision (signed after the 404 escalation for a private repo, anonymous for a public one), and a mid-negotiation denial surfaces through the sanitized error path rather than reading as an empty or successful fetch. The receive-pack path is unchanged. Covers the deadlock repro, multi-round accumulation on the wire, per-round signing for private and public fetches, mid-negotiation denial surfacing, the withheld-shaped forwarding path, and single-round non-regression. --- crates/git-remote-gitlawb/src/main.rs | 674 ++++++++++++++++++++++++-- 1 file changed, 638 insertions(+), 36 deletions(-) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index ca46c9ce..49b3f9fe 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -276,27 +276,32 @@ fn handle_connect( // ── Phase 2: pack exchange (POST /) ────────────────────────────── // - // The two services behave differently with their write pipe: + // The two services frame Phase 2 differently: // // git-upload-pack (clone/fetch): - // Client sends pkt-line want/have negotiation ending with "done\n", - // but does NOT close its write pipe — it waits for the pack response. - // We must detect the terminal "done\n" pkt-line to know when to POST. + // Git speaks the stateful native protocol over `connect`: it sends its + // wants, a flush, then have-batches each terminated by a flush, and blocks + // for the server's ACK/NAK after every flush before it sends more haves or + // the terminal "done\n". The node serves `git upload-pack --stateless-rpc`, + // which keeps no state between POSTs, so we run a per-round loop: POST a + // self-contained request once per flush-terminated batch and stream each + // response back to git, until the "done" round returns the pack. Collapsing + // this into one POST deadlocks a multi-round fetch (#117). // // git-receive-pack (push): - // Client sends ref-update commands + complete PACK blob, then closes - // its write pipe. read_to_end is safe and correct here. + // Git sends ref-update commands + the complete PACK blob, then closes its + // write pipe. read_to_end is safe and correct here, a single POST. - let request_body = if service == "git-upload-pack" { - read_upload_pack_request(stdin).context("reading upload-pack request")? - } else { - let mut buf = Vec::new(); - stdin - .read_to_end(&mut buf) - .context("reading receive-pack request")?; - buf - }; + let post_url = format!("{}/{}", repo_base, service); + if service == "git-upload-pack" { + return negotiate_upload_pack(&client, &post_url, service, signing_key, stdin, &mut stdout); + } + + let mut request_body = Vec::new(); + stdin + .read_to_end(&mut request_body) + .context("reading receive-pack request")?; tracing::debug!("pack request: {} bytes from git", request_body.len()); if request_body.is_empty() { @@ -305,9 +310,7 @@ fn handle_connect( return Ok(()); } - let post_url = format!("{}/{}", repo_base, service); tracing::debug!("POST {post_url} ({} bytes)", request_body.len()); - let req = build_pack_post_request(&client, &post_url, service, &request_body, signing_key); // Attach the body after signing so the pack bytes are moved, not cloned — @@ -424,33 +427,48 @@ fn parse_gitlawb_url(url: &str) -> Result<(String, String, String)> { Ok((did_string.to_string(), short_owner, repo_name)) } -/// Read a complete git-upload-pack request from the pkt-line stream. -/// -/// For upload-pack, git sends its want/have negotiation ending with the pkt-line -/// `"done\n"` but does NOT close its write pipe afterwards — it waits for the -/// server's pack response. We detect the terminal "done\n" and stop reading. +/// How a single upload-pack negotiation round ended. +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +enum RoundEnd { + /// A flush pkt (`0000`) ended the round; more negotiation may follow. + Flush, + /// The terminal `done\n` pkt ended the round; the pack follows. + Done, + /// The stream ended (git closed its write pipe). + Eof, +} + +/// Read ONE round of git's upload-pack request from the pkt-line stream. /// -/// We also handle the flush-only case (`"0000"`) that git sends when it already -/// has everything it needs (up-to-date clone). -fn read_upload_pack_request(stdin: &mut R) -> Result> { - let mut buf = Vec::new(); +/// Git speaks the stateful native protocol over the `connect` capability: it +/// sends its `want` lines, a flush, then `have` batches each terminated by a +/// flush, and blocks for the server's ACK/NAK after every flush before sending +/// more haves or the terminal `done\n`. Each call returns one such round: the +/// pkt-lines up to (but not including) the terminating flush or `done`, plus +/// which terminator ended it. `negotiate_upload_pack` reassembles these into +/// self-contained stateless-RPC POST bodies. Returning at the flush, instead of +/// buffering past it waiting for `done`, is what lets a multi-round fetch make +/// progress rather than deadlock (#117). +fn read_upload_pack_round(stdin: &mut R) -> Result<(Vec, RoundEnd)> { + let mut content = Vec::new(); loop { - // Read the 4-byte hex pkt-line length prefix + // Read the 4-byte hex pkt-line length prefix. let mut len_bytes = [0u8; 4]; match stdin.read_exact(&mut len_bytes) { Ok(_) => {} - Err(e) if e.kind() == io::ErrorKind::UnexpectedEof => break, + Err(e) if e.kind() == io::ErrorKind::UnexpectedEof => { + return Ok((content, RoundEnd::Eof)); + } Err(e) => return Err(e.into()), } - buf.extend_from_slice(&len_bytes); let len_hex = std::str::from_utf8(&len_bytes).unwrap_or("0000"); let pkt_len = usize::from_str_radix(len_hex, 16).unwrap_or(0); if pkt_len == 0 { - // Flush pkt "0000" — keep buffering (more pkt-lines may follow) - continue; + // Flush pkt "0000": this round is complete. + return Ok((content, RoundEnd::Flush)); } if pkt_len < 4 { @@ -462,16 +480,140 @@ fn read_upload_pack_request(stdin: &mut R) -> Result> { stdin .read_exact(&mut data) .context("reading pkt-line data")?; - buf.extend_from_slice(&data); - // "done\n" signals the end of the want/have negotiation + // "done\n" ends the negotiation; the pack follows. The terminator is + // reported out-of-band and re-emitted by the caller, so it is not + // appended to `content`. if data == b"done\n" { - tracing::debug!("upload-pack: got 'done', request complete"); - break; + tracing::debug!("upload-pack: got 'done', round complete"); + return Ok((content, RoundEnd::Done)); + } + + content.extend_from_slice(&len_bytes); + content.extend_from_slice(&data); + } +} + +/// POST one self-contained stateless-RPC request body and stream the response to +/// `stdout`. Signs the request when `signing_key` is present, so every round of a +/// private-repo fetch carries the caller's signature (the node visibility-gates +/// each POST at "/"). A non-2xx status surfaces through the sanitized error path +/// rather than being rendered as an empty or successful fetch (INV-6/INV-8). +fn post_pack_round( + client: &reqwest::blocking::Client, + post_url: &str, + service: &str, + signing_key: Option<&Keypair>, + body: Vec, + stdout: &mut impl Write, +) -> Result<()> { + tracing::debug!("POST {post_url} ({} bytes)", body.len()); + let req = build_pack_post_request(client, post_url, service, &body, signing_key); + // Attach the body after signing so the bytes are moved, not cloned. + let resp = req + .body(body) + .send() + .with_context(|| format!("POST {post_url}"))?; + + if !resp.status().is_success() { + let status = resp.status(); + let err_body = read_error_body(resp); + let path = format!("/{service}"); + bail!( + "{}", + http_error_message("POST", &path, status, &err_body, None) + ); + } + + let bytes = resp.bytes().context("reading pack response")?; + tracing::debug!("pack response: {} bytes from node", bytes.len()); + stdout.write_all(&bytes)?; + stdout.flush()?; + Ok(()) +} + +/// Drive a git-upload-pack fetch as a v0 smart-HTTP stateless-RPC client. +/// +/// Git (over `connect`) speaks the stateful native protocol; the node serves +/// `git upload-pack --stateless-rpc`, which keeps no state between POSTs. Bridge +/// the two: capture the want block once, then for each flush-terminated have +/// batch POST a self-contained request (the wants plus every have accumulated so +/// far, then exactly one terminator) and stream the ACK/NAK back to git so it can +/// continue. The `done` round returns the pack. Every POST re-sends the wants and +/// all prior haves because the server is stateless; leaving an intermediate flush +/// in the body would truncate the negotiation, so each body carries exactly one +/// terminator. +fn negotiate_upload_pack( + client: &reqwest::blocking::Client, + post_url: &str, + service: &str, + signing_key: Option<&Keypair>, + stdin: &mut R, + stdout: &mut impl Write, +) -> Result<()> { + // Opening round: the want block, a bare `done`, or an empty/flush-only + // request from an up-to-date or aborted fetch. + let (wants, wend) = read_upload_pack_round(stdin)?; + match wend { + // Up-to-date with an empty request, truncated, or aborted before a + // terminator: nothing safe to POST. + RoundEnd::Eof => return Ok(()), + // A `done` with no preceding want-section flush: the bare-`done` request + // the single-round tests feed. Forward it verbatim as one POST. + RoundEnd::Done => { + let mut body = wants; + body.extend_from_slice(b"0009done\n"); + return post_pack_round(client, post_url, service, signing_key, body, stdout); } + // Normal: the want section ended with its flush; negotiate the haves. + // (An empty want block here is a flush-only opener and falls through to + // the loop, which POSTs nothing and returns at EOF.) + RoundEnd::Flush => {} } - Ok(buf) + // The node is stateless between POSTs, so re-send the wants and every have so + // far in each round. + let mut acc_haves: Vec = Vec::new(); + loop { + let (batch, bend) = read_upload_pack_round(stdin)?; + + if batch.is_empty() && bend == RoundEnd::Eof { + // git closed with no new haves (clone with wants+flush+done is handled + // by the Done arm below; this is an up-to-date flush-only opener or a + // clean abort): nothing more to POST. + return Ok(()); + } + + acc_haves.extend_from_slice(&batch); + + // Compose a self-contained stateless-RPC request: wants + one flush + all + // accumulated haves + exactly one terminator. No intermediate flush + // survives into the body, or the stateless server treats the first flush + // after the haves as end-of-request and truncates the negotiation. + let mut body = wants.clone(); + body.extend_from_slice(b"0000"); + body.extend_from_slice(&acc_haves); + + let final_round = match bend { + RoundEnd::Flush => { + body.extend_from_slice(b"0000"); + false + } + // A `done` round is final. A nonempty EOF-terminated batch is treated + // as final too (a test-shim convenience; a real pipe ends a fetch with + // `done`, and a genuine mid-negotiation EOF means git aborted). + RoundEnd::Done | RoundEnd::Eof => { + body.extend_from_slice(b"0009done\n"); + true + } + }; + + post_pack_round(client, post_url, service, signing_key, body, stdout)?; + + if final_round { + return Ok(()); + } + } } /// Strip the HTTP smart-protocol service announcement from a GET /info/refs response. @@ -1093,6 +1235,35 @@ mod tests { "a tampered @path must fail verification" ); } + + // #117: a multi-round POST body (wants + flush + accumulated haves + done) + // signs and verifies over its EXACT bytes. Each per-round POST a private + // fetch emits is a different accumulated body, and build_pack_post_request + // signs the same bytes it sends, so every round is independently accepted + // by the node verifier, not just the single-round `done` body. + let mut acc = Vec::new(); + acc.extend_from_slice(&pkt(&format!( + "want {} multi_ack_detailed side-band-64k\n", + "a".repeat(40) + ))); + acc.extend_from_slice(b"0000"); + acc.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + acc.extend_from_slice(&pkt(&format!("have {}\n", "2".repeat(40)))); + acc.extend_from_slice(&pkt("done\n")); + let post_url = "http://node.example/zOwner/myrepo/git-upload-pack"; + let req = build_pack_post_request(&client, post_url, "git-upload-pack", &acc, Some(&kp)) + .body(acc.clone()) + .build() + .unwrap(); + node_verifies( + "POST", + &path_and_query(&req), + &acc, + &header(&req, "signature-input"), + &header(&req, "signature"), + &header(&req, "content-digest"), + ) + .expect("a multi-round accumulated POST body must verify under the node's verifier"); } #[test] @@ -1316,4 +1487,435 @@ mod tests { assert!(help.contains("GITLAWB_NODE")); assert!(help.ends_with('\n')); } + + // ── #117 multi-round fetch negotiation ─────────────────────────────────── + + /// Encode a git pkt-line: 4-byte hex length (incl. the 4 bytes) + data. + fn pkt(data: &str) -> Vec { + format!("{:04x}{}", data.len() + 4, data).into_bytes() + } + + /// A realistic capability-bearing opening want line (git puts its capability + /// list on the first want), so the multi-round tests exercise the want shape + /// real git actually sends, not a bare `want `. + fn want_line(sha: &str) -> Vec { + pkt(&format!( + "want {sha} multi_ack_detailed side-band-64k ofs-delta agent=git/2.43\n" + )) + } + + /// A reader that yields `seed`, then BLOCKS until `gate` fires. Models git + /// holding the upload-pack pipe open after a have-batch flush, waiting for the + /// server's ACK/NAK before it sends more. A real pipe blocks here; an in-memory + /// Cursor would EOF and hide the bug, which is why the pre-#117 tests missed it. + struct BlockAfterSeed { + seed: io::Cursor>, + gate: std::sync::mpsc::Receiver<()>, + } + impl Read for BlockAfterSeed { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + let n = self.seed.read(buf)?; + if n > 0 { + return Ok(n); + } + let _ = self.gate.recv(); // block like a live pipe; unblock -> EOF + Ok(0) + } + } + + /// #117 primitive contract (matrix item 1): `read_upload_pack_round` returns at + /// a flush-terminated batch instead of buffering past it waiting for `done`. + /// The pre-#117 `read_upload_pack_request` blocked here (the deadlock). A + /// blocking reader proves the return under live-pipe semantics: if the reader + /// buffered past the flush it would block on the gate and the test would time out. + #[test] + fn read_upload_pack_round_returns_on_flush_without_done() { + let mut seed = Vec::new(); + seed.extend_from_slice(&want_line(&"a".repeat(40))); + seed.extend_from_slice(b"0000"); // end of wants + for i in 0..32u32 { + seed.extend_from_slice(&pkt(&format!("have {:040x}\n", i))); + } + seed.extend_from_slice(b"0000"); // have-batch flush; git awaits ACK, sends NO done + + let (tx, rx) = std::sync::mpsc::channel::<()>(); + let mut reader = BlockAfterSeed { + seed: io::Cursor::new(seed), + gate: rx, + }; + + let (done_tx, done_rx) = std::sync::mpsc::channel::<(Vec, RoundEnd)>(); + let handle = std::thread::spawn(move || { + // Opening round (wants) returns at the first flush; the have batch + // returns at the second flush. Neither may block for a `done`. + let _wants = read_upload_pack_round(&mut reader).unwrap(); + let haves = read_upload_pack_round(&mut reader).unwrap(); + let _ = done_tx.send(haves); + }); + + let got = done_rx.recv_timeout(std::time::Duration::from_secs(2)); + let _ = tx.send(()); // unblock the reader so the thread can exit + let _ = handle.join(); + + let (batch, end) = got.expect( + "read_upload_pack_round blocked past a flush-terminated have-batch \ + waiting for `done` (multi-round fetch deadlock #117)", + ); + assert_eq!(end, RoundEnd::Flush, "a flush terminates the round"); + assert!( + batch.windows(4).any(|w| w == b"have"), + "the have batch content is returned to the caller" + ); + } + + /// #117 core (matrix item 2): a multi-round negotiation issues MORE THAN ONE + /// POST, and each POST body is a self-contained stateless-RPC request: wants + + /// one flush + every have accumulated so far + exactly one terminator. The + /// round-2 body is asserted byte-for-byte, which pins both accumulation (round + /// 1's have is present) and the one-terminator invariant (no intermediate flush + /// survives). Pre-#117 this was a single POST. + #[test] + fn multi_round_fetch_posts_each_round_with_accumulated_wants_and_haves() { + let want_sha = "a".repeat(40); + let have1 = "1".repeat(40); + let have2 = "2".repeat(40); + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + // Round 1 (flush-terminated): an ACK/NAK continuation, no pack yet. + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\n", + }, + // Round 2 (done): final response plus the (fake) pack. + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + + let wants = want_line(&want_sha); + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&wants); + stdin_bytes.extend_from_slice(b"0000"); // end of wants + stdin_bytes.extend_from_slice(&pkt(&format!("have {have1}\n"))); + stdin_bytes.extend_from_slice(b"0000"); // round-1 flush: git awaits ACK + stdin_bytes.extend_from_slice(&pkt(&format!("have {have2}\n"))); + stdin_bytes.extend_from_slice(&pkt("done\n")); // round-2 terminator + + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("multi-round fetch should complete"); + + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 3, + "GET + two POSTs, one per negotiation round" + ); + assert!( + request_body(&requests[1]) + .windows(want_sha.len()) + .any(|w| w == want_sha.as_bytes()), + "round-1 POST must carry the wants" + ); + + // Round-2 body: wants + one flush + BOTH haves + done, with no flush + // between the haves (the accumulation + one-terminator invariant). + let mut expected = Vec::new(); + expected.extend_from_slice(&wants); + expected.extend_from_slice(b"0000"); + expected.extend_from_slice(&pkt(&format!("have {have1}\n"))); + expected.extend_from_slice(&pkt(&format!("have {have2}\n"))); + expected.extend_from_slice(&pkt("done\n")); + assert_eq!( + request_body(&requests[2]), + expected, + "round-2 POST body must be wants + flush + all accumulated haves + one done terminator" + ); + } + + /// Extract the HTTP request body (bytes after the header/body separator) from a + /// captured request string. pkt-lines are ASCII, so the lossy round-trip is exact. + fn request_body(request: &str) -> Vec { + match request.split_once("\r\n\r\n") { + Some((_, body)) => body.as_bytes().to_vec(), + None => Vec::new(), + } + } + + /// U4 / INV-8 / INV-12: a private-repo multi-round fetch escalates on the 404 + /// exactly once, then EVERY per-round POST carries the signature. Must-not: no + /// round goes out unsigned (an unsigned round 2 would 404 the owner's own fetch). + #[test] + fn private_multi_round_signs_every_post() { + let kp = Keypair::generate(); + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "404 Not Found", + body: r#"{"message":"not found"}"#, + }, + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\n", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(two_round_stdin()); + handle_connect(&repo_base, "git-upload-pack", Some(&kp), &mut stdin) + .expect("private multi-round fetch should complete"); + + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 4, + "anon GET, signed GET retry, then two signed POSTs" + ); + assert!( + !requests[0].to_lowercase().contains("signature-input"), + "first GET is anonymous" + ); + assert!( + requests[1].to_lowercase().contains("signature-input"), + "GET retry is signed" + ); + assert!( + requests[2].to_lowercase().contains("signature-input"), + "round-1 POST is signed" + ); + assert!( + requests[3].to_lowercase().contains("signature-input"), + "round-2 POST is signed: no per-round POST may go out unsigned for a private repo" + ); + } + + /// U4: a public multi-round fetch stays anonymous on EVERY round even with a + /// keypair present. Must-not: no round signs (no DID disclosure on a public fetch). + #[test] + fn public_multi_round_stays_anonymous_every_post() { + let kp = Keypair::generate(); + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\n", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(two_round_stdin()); + handle_connect(&repo_base, "git-upload-pack", Some(&kp), &mut stdin) + .expect("public multi-round fetch should complete"); + + let requests = server.join().unwrap(); + assert_eq!(requests.len(), 3, "GET + two POSTs, no retry"); + for (i, r) in requests.iter().enumerate() { + assert!( + !r.to_lowercase().contains("signature-input"), + "request {i} must stay anonymous on a public fetch" + ); + } + } + + /// U4 / INV-8 / INV-12: a denial mid-negotiation (round-2 POST 404) surfaces as + /// an error, not a silent empty/successful fetch, and does NOT re-trigger the + /// Phase-1 signed-retry escalation (that is a GET-only, Phase-1-only behavior). + #[test] + fn mid_negotiation_post_denial_surfaces_without_reescalation() { + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\n", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "404 Not Found", + body: r#"{"message":"repository is no longer readable"}"#, + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(two_round_stdin()); + let err = handle_connect(&repo_base, "git-upload-pack", None, &mut stdin).unwrap_err(); + let message = err.to_string(); + assert!( + message.contains("POST /git-upload-pack returned 404 Not Found"), + "the mid-negotiation denial must surface, not read as an empty/successful fetch" + ); + assert!(message.contains("repository is no longer readable")); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 3, + "GET + two POSTs, then bail: a POST denial must not re-run the Phase-1 escalation" + ); + } + + /// Two-round upload-pack request: wants, flush, have1, flush (round 1), have2, + /// done (round 2). Shared by the U4 signing/denial tests. + fn two_round_stdin() -> Vec { + let mut v = Vec::new(); + v.extend_from_slice(&want_line(&"a".repeat(40))); + v.extend_from_slice(b"0000"); + v.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + v.extend_from_slice(b"0000"); + v.extend_from_slice(&pkt(&format!("have {}\n", "2".repeat(40)))); + v.extend_from_slice(&pkt("done\n")); + v + } + + /// U5 (matrix item 5, plumbing only): the node's withheld-blob path + /// (`upload_pack_excluding`) ignores negotiation and answers the first POST + /// with NAK plus a full self-contained pack. The helper must forward that + /// response and terminate without hanging. Whether REAL git accepts a pack + /// where it expected an ACK continuation is U7's real-git scenario, not this + /// mock (a Cursor never reacts to the response). + #[test] + fn withheld_shaped_response_is_forwarded_without_hanging() { + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + // Withheld shape: NAK + full pack on the FIRST POST, negotiation ignored. + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfullselfcontainedpack", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + // A single flush-terminated round then EOF: the helper POSTs once, forwards + // the NAK+pack, and terminates at EOF rather than looping forever. + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&want_line(&"a".repeat(40))); + stdin_bytes.extend_from_slice(b"0000"); + stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + stdin_bytes.extend_from_slice(b"0000"); + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("withheld-shaped fetch should forward the pack and terminate"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 2, + "GET + one POST; the forwarded pack does not hang the helper" + ); + } + + /// U6 (matrix item 6): a fresh clone (wants, flush, done) issues exactly ONE POST. + #[test] + fn fresh_clone_issues_single_post() { + let want_sha = "a".repeat(40); + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&want_line(&want_sha)); + stdin_bytes.extend_from_slice(b"0000"); + stdin_bytes.extend_from_slice(&pkt("done\n")); + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("clone should complete"); + let requests = server.join().unwrap(); + assert_eq!(requests.len(), 2, "fresh clone is exactly one POST"); + assert!(request_body(&requests[1]) + .windows(want_sha.len()) + .any(|w| w == want_sha.as_bytes())); + } + + /// U6 (matrix item 6): an up-to-date / flush-only opener issues ZERO POSTs and + /// completes without entering an ACK wait. + #[test] + fn flush_only_opener_skips_post() { + let (base_url, server) = serve_http(vec![TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(b"0000".to_vec()); // flush only, nothing to fetch + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("up-to-date fetch should complete"); + let requests = server.join().unwrap(); + assert_eq!(requests.len(), 1, "flush-only opener issues no POST"); + } + + /// U6 (matrix item 6): a single-shot fetch git resolved in one round (wants, + /// flush, haves, done, no intermediate flush) issues exactly ONE POST. Must-not: + /// the fix must not split a single-shot negotiation into multiple POSTs. + #[test] + fn single_shot_fetch_is_one_post() { + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&want_line(&"a".repeat(40))); + stdin_bytes.extend_from_slice(b"0000"); + stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "2".repeat(40)))); + stdin_bytes.extend_from_slice(&pkt("done\n")); // done with no intermediate flush + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("fetch should complete"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 2, + "a single-shot negotiation must be exactly one POST, not split" + ); + } } From e02ea757a7753ca6fe694fd5db876065caba2271 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 11 Jul 2026 19:46:39 -0500 Subject: [PATCH 2/4] test(git-remote): real-git integration proof of the multi-round fetch loop (#117) A committed integration test drives a real `git fetch` through the built helper against a real `git upload-pack --stateless-rpc`, so the stateful-to-stateless ACK bridging is proven by execution rather than reasoned. The main.rs mock tests cannot falsify it: a pre-scripted Cursor never reacts to the server's ACKs. An in-test shim replicates the node's v0 serving; the fetch is forced to at least two negotiation rounds (a fixture that resolved in one round fails the test) and the resulting object graph is verified. The second scenario drives the withheld-blob shape (a full pack on the first POST, as upload_pack_excluding does) through real git and records the outcome: real git rejects a pack where it expected an ACK continuation ("expected ACK/NAK, got ..."), so a multi-round fetch of a withheld repo does not complete. The helper forwards and terminates cleanly rather than hanging, so this is a node-side concern, not a helper defect, left as a follow-up per the plan's Withheld-Path Decision. The test guards the helper's non-hang behavior and surfaces the break if the assumption ever changes. --- .../tests/real_git_fetch.rs | 436 ++++++++++++++++++ 1 file changed, 436 insertions(+) create mode 100644 crates/git-remote-gitlawb/tests/real_git_fetch.rs diff --git a/crates/git-remote-gitlawb/tests/real_git_fetch.rs b/crates/git-remote-gitlawb/tests/real_git_fetch.rs new file mode 100644 index 00000000..094762b0 --- /dev/null +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -0,0 +1,436 @@ +//! #117 end-to-end: drive a REAL `git fetch` through the built helper against a +//! REAL `git upload-pack --stateless-rpc`, so the stateful-to-stateless bridging +//! is proven by execution, not reasoned. The unit tests in `main.rs` use a +//! pre-scripted Cursor and a canned HTTP mock, which cannot tell whether real git +//! (a stateful client over `connect`) actually parses the stateless-RPC server's +//! ACK responses and converges. That is the one load-bearing bet in the fix, and +//! only this test can falsify it. +//! +//! The in-test shim replicates the node's v0 smart-HTTP serving +//! (`gitlawb-node/src/git/smart_http.rs`): the info/refs advertisement wrapped in +//! the `# service=` pkt-line + flush, and each POST piped to +//! `git upload-pack --stateless-rpc`. The node crate's own tests already require +//! git, so committing this always-on is consistent with the suite. + +use std::io::{BufRead, BufReader, Read, Write}; +use std::net::{TcpListener, TcpStream}; +use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::Arc; +use std::time::{Duration, Instant}; + +/// git pkt-line: 4-byte hex length (incl. the 4 bytes) + data. +fn pkt(data: &[u8]) -> Vec { + let mut out = format!("{:04x}", data.len() + 4).into_bytes(); + out.extend_from_slice(data); + out +} + +fn unique_dir(tag: &str) -> PathBuf { + static COUNTER: AtomicUsize = AtomicUsize::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!("gitlawb-u7-{}-{}-{}", tag, std::process::id(), n)); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +/// Run git in `dir` with deterministic identity/config, asserting success. +fn git(dir: &Path, args: &[&str]) -> Vec { + let out = Command::new("git") + .args([ + "-c", + "user.name=t", + "-c", + "user.email=t@example.invalid", + "-c", + "commit.gpgsign=false", + "-c", + "init.defaultBranch=main", + "-c", + "protocol.version=2", + ]) + .arg("-C") + .arg(dir) + .args(args) + .output() + .unwrap_or_else(|e| panic!("failed to spawn git {args:?}: {e}")); + assert!( + out.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + out.stdout +} + +/// Run `git upload-pack --stateless-rpc [--advertise-refs] `, optionally +/// feeding `input` on stdin. Returns raw stdout (the wire bytes). +fn upload_pack(repo: &Path, advertise: bool, input: &[u8]) -> Vec { + let mut cmd = Command::new("git"); + cmd.arg("upload-pack").arg("--stateless-rpc"); + if advertise { + cmd.arg("--advertise-refs"); + } + cmd.arg(repo) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + let mut child = cmd.spawn().expect("spawn git upload-pack"); + let mut stdin = child.stdin.take().unwrap(); + let input = input.to_vec(); + let writer = std::thread::spawn(move || { + let _ = stdin.write_all(&input); + // drop closes stdin + }); + let out = child.wait_with_output().expect("wait upload-pack"); + writer.join().ok(); + assert!( + out.status.success(), + "git upload-pack failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + out.stdout +} + +#[derive(Clone, Copy)] +enum ShimMode { + /// Faithful v0 serving: pipe each POST to `git upload-pack --stateless-rpc`. + Normal, + /// The withheld-blob shape: ignore negotiation and answer the FIRST POST with + /// a full self-contained pack (as `upload_pack_excluding` does on the node). + WithheldFirstPost, +} + +struct Shim { + base_url: String, + posts: Arc, + stop: Arc, + handle: Option>, +} + +impl Drop for Shim { + fn drop(&mut self) { + self.stop.store(true, Ordering::SeqCst); + // Nudge the accept loop with a throwaway connection so it observes `stop`. + if let Ok(addr) = self + .base_url + .trim_start_matches("http://") + .parse::() + { + let _ = TcpStream::connect_timeout(&addr, Duration::from_millis(200)); + } + if let Some(h) = self.handle.take() { + let _ = h.join(); + } + } +} + +/// Start a minimal smart-HTTP shim serving `repo` on 127.0.0.1. Handles the +/// upload-pack advertisement (GET) and pack negotiation (POST), counting POSTs. +fn start_shim(repo: PathBuf, mode: ShimMode) -> Shim { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + listener.set_nonblocking(true).unwrap(); + let addr = listener.local_addr().unwrap(); + let base_url = format!("http://{addr}"); + let posts = Arc::new(AtomicUsize::new(0)); + let stop = Arc::new(AtomicBool::new(false)); + + let posts_t = posts.clone(); + let stop_t = stop.clone(); + let handle = std::thread::spawn(move || { + while !stop_t.load(Ordering::SeqCst) { + match listener.accept() { + Ok((stream, _)) => { + handle_conn(stream, &repo, mode, &posts_t); + } + Err(ref e) if e.kind() == std::io::ErrorKind::WouldBlock => { + std::thread::sleep(Duration::from_millis(5)); + } + Err(_) => break, + } + } + }); + + Shim { + base_url, + posts, + stop, + handle: Some(handle), + } +} + +fn handle_conn(stream: TcpStream, repo: &Path, mode: ShimMode, posts: &AtomicUsize) { + stream.set_read_timeout(Some(Duration::from_secs(30))).ok(); + let mut reader = BufReader::new(stream); + + // Request line. + let mut request_line = String::new(); + if reader.read_line(&mut request_line).unwrap_or(0) == 0 { + return; + } + let mut parts = request_line.split_whitespace(); + let method = parts.next().unwrap_or("").to_string(); + let target = parts.next().unwrap_or("").to_string(); + + // Headers. + let mut content_length = 0usize; + loop { + let mut line = String::new(); + if reader.read_line(&mut line).unwrap_or(0) == 0 { + break; + } + if line == "\r\n" || line == "\n" { + break; + } + if let Some((name, value)) = line.split_once(':') { + if name.eq_ignore_ascii_case("content-length") { + content_length = value.trim().parse().unwrap_or(0); + } + } + } + + // Body. + let mut body = vec![0u8; content_length]; + if content_length > 0 { + reader.read_exact(&mut body).ok(); + } + + let (content_type, payload) = if method == "GET" && target.contains("/info/refs") { + // v0 advertisement, wrapped exactly as the node's info_refs does. + let adv = upload_pack(repo, true, b""); + let mut wrapped = pkt(b"# service=git-upload-pack\n"); + wrapped.extend_from_slice(b"0000"); + wrapped.extend_from_slice(&adv); + ("application/x-git-upload-pack-advertisement", wrapped) + } else if method == "POST" && target.ends_with("/git-upload-pack") { + let n = posts.fetch_add(1, Ordering::SeqCst); + let out = match mode { + ShimMode::Normal => upload_pack(repo, false, &body), + ShimMode::WithheldFirstPost if n == 0 => full_pack_response(repo), + ShimMode::WithheldFirstPost => upload_pack(repo, false, &body), + }; + ("application/x-git-upload-pack-result", out) + } else { + write_response(reader.into_inner(), "404 Not Found", "text/plain", b"no"); + return; + }; + + write_response(reader.into_inner(), "200 OK", content_type, &payload); +} + +/// The `upload_pack_excluding` shape: NAK plus a full self-contained pack, +/// negotiation ignored. Built by asking a real upload-pack for the tip with no +/// haves (want + done), which yields exactly `NAK` + the full pack. +fn full_pack_response(repo: &Path) -> Vec { + let head = String::from_utf8(git(repo, &["rev-parse", "HEAD"])).unwrap(); + let head = head.trim(); + let mut req = + pkt(format!("want {head} multi_ack_detailed side-band-64k ofs-delta\n").as_bytes()); + req.extend_from_slice(b"0000"); + req.extend_from_slice(&pkt(b"done\n")); + upload_pack(repo, false, &req) +} + +fn write_response(mut stream: TcpStream, status: &str, content_type: &str, body: &[u8]) { + let header = format!( + "HTTP/1.1 {status}\r\nContent-Type: {content_type}\r\nContent-Length: {}\r\nCache-Control: no-cache\r\nConnection: close\r\n\r\n", + body.len() + ); + let _ = stream.write_all(header.as_bytes()); + let _ = stream.write_all(body); + let _ = stream.flush(); +} + +/// Run `git fetch` in `clone` through the helper, with a hard timeout so a +/// regression to the deadlock fails fast instead of hanging the suite. +fn fetch_with_helper(clone: &Path, node_url: &str) -> (bool, std::process::Output) { + let helper_bin = PathBuf::from(env!("CARGO_BIN_EXE_git-remote-gitlawb")); + let helper_dir = helper_bin.parent().unwrap().to_path_buf(); + let path_env = match std::env::var_os("PATH") { + Some(p) => { + let mut dirs = vec![helper_dir.clone()]; + dirs.extend(std::env::split_paths(&p)); + std::env::join_paths(dirs).unwrap() + } + None => helper_dir.clone().into_os_string(), + }; + + let mut child = Command::new("git") + .args(["-c", "protocol.version=2"]) + .arg("-C") + .arg(clone) + .args(["fetch", "origin", "main"]) + .env("PATH", path_env) + .env("GITLAWB_NODE", node_url) + .env("GITLAWB_KEY", "/nonexistent-key-for-anon-fetch") + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn git fetch"); + + let deadline = Instant::now() + Duration::from_secs(30); + loop { + if let Some(_status) = child.try_wait().unwrap() { + let out = child.wait_with_output().unwrap(); + return (true, out); + } + if Instant::now() >= deadline { + let _ = child.kill(); + let out = child.wait_with_output().unwrap(); + return (false, out); // timed out: the deadlock signature + } + std::thread::sleep(Duration::from_millis(50)); + } +} + +/// Build a server repo with a shared history deep enough to force multi-round +/// negotiation (>~32 haves), plus a clone of it, then advance the server so the +/// fetch has something to negotiate. Returns (server, clone). +fn build_divergent_repos(shared_commits: usize) -> (PathBuf, PathBuf) { + let server = unique_dir("server"); + git(&server, &["init", "-q"]); + std::fs::write(server.join("base.txt"), b"base").unwrap(); + git(&server, &["add", "."]); + git(&server, &["commit", "-q", "-m", "base"]); + // A deep shared history the clone will offer as haves. + for i in 0..shared_commits { + git( + &server, + &[ + "commit", + "-q", + "--allow-empty", + "-m", + &format!("shared-{i}"), + ], + ); + } + + let clone = unique_dir("clone"); + // Clone over file:// so the clone shares the full history. + let status = Command::new("git") + .args(["clone", "-q"]) + .arg(&server) + .arg(&clone) + .output() + .unwrap(); + assert!( + status.status.success(), + "clone failed: {}", + String::from_utf8_lossy(&status.stderr) + ); + + // Advance the server so the fetch must transfer new objects. + for i in 0..3 { + git( + &server, + &[ + "commit", + "-q", + "--allow-empty", + "-m", + &format!("server-{i}"), + ], + ); + } + + // Point the clone's origin at the gitlawb:// scheme so git invokes the helper. + git( + &clone, + &[ + "remote", + "set-url", + "origin", + "gitlawb://did:key:zTESTOWNER/myrepo", + ], + ); + (server, clone) +} + +/// Matrix item 7, bridging half: a real multi-round `git fetch` through the +/// helper against a real `git upload-pack --stateless-rpc` completes, is observed +/// at >=2 POSTs (the executable form of the trigger; a fixture that resolved in +/// one round would fail this), and produces a correct object graph. +#[test] +fn real_git_multi_round_fetch_completes() { + let (server, clone) = build_divergent_repos(50); + let shim = start_shim(server.clone(), ShimMode::Normal); + + let (completed, out) = fetch_with_helper(&clone, &shim.base_url); + let posts = shim.posts.load(Ordering::SeqCst); + + assert!( + completed, + "git fetch did not complete within the timeout (deadlock signature). stderr:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + out.status.success(), + "git fetch failed. stderr:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + posts >= 2, + "fixture did not force multi-round negotiation (observed {posts} POST(s)); the bridging path was not exercised" + ); + + // The fetched tip is present and the clone's object graph is intact. + let server_head = String::from_utf8(git(&server, &["rev-parse", "HEAD"])).unwrap(); + let fetched = String::from_utf8(git(&clone, &["rev-parse", "FETCH_HEAD"])).unwrap(); + assert_eq!( + server_head.trim(), + fetched.trim(), + "FETCH_HEAD must match the server tip" + ); + git(&clone, &["fsck", "--full"]); + + cleanup(&[server, clone]); +} + +/// Matrix item 7, withheld half (the Withheld-Path Decision gate): the node's +/// `upload_pack_excluding` answers the FIRST POST with NAK plus a full pack, +/// mid-negotiation. Drive that shape through REAL git and record whether it +/// accepts a pack where it expected an ACK continuation. If real git rejects it, +/// this test captures the break mode that the decision's remedy addresses. +#[test] +fn real_git_withheld_shaped_first_post() { + let (server, clone) = build_divergent_repos(50); + let shim = start_shim(server.clone(), ShimMode::WithheldFirstPost); + + let (completed, out) = fetch_with_helper(&clone, &shim.base_url); + let posts = shim.posts.load(Ordering::SeqCst); + let stderr = String::from_utf8_lossy(&out.stderr).to_string(); + + // The helper itself must not hang or panic regardless of git's verdict. + assert!( + completed, + "helper hung on a withheld-shaped response (should forward-and-terminate, not deadlock). stderr:\n{stderr}" + ); + + if out.status.success() { + // Real git accepted the mid-negotiation pack: the withheld multi-round + // path works end to end with no extra handling. + let server_head = String::from_utf8(git(&server, &["rev-parse", "HEAD"])).unwrap(); + let fetched = String::from_utf8(git(&clone, &["rev-parse", "FETCH_HEAD"])).unwrap(); + assert_eq!(server_head.trim(), fetched.trim()); + git(&clone, &["fsck", "--full"]); + } else { + // Real git rejected the mid-negotiation pack. This is a genuine outcome, + // not a helper defect (the helper forwarded and terminated cleanly). The + // Withheld-Path Decision's remedy owns the fix; this branch documents the + // observed break so a regression in the assumption is visible in CI. + eprintln!( + "WITHHELD-PATH NOTE (#117): real git did NOT accept a NAK+pack mid-negotiation \ + (observed {posts} POST(s)). Per the Withheld-Path Decision this routes to a node-side \ + follow-up or an accepted withheld=full-clone limitation, not helper code. git stderr:\n{stderr}" + ); + } + + cleanup(&[server, clone]); +} + +fn cleanup(dirs: &[PathBuf]) { + for d in dirs { + let _ = std::fs::remove_dir_all(d); + } +} From bd05407880a71f182a853b85b15e93fa235e1f5f Mon Sep 17 00:00:00 2001 From: t Date: Sat, 11 Jul 2026 19:53:24 -0500 Subject: [PATCH 3/4] fix(git-remote): skip content-free flush rounds in the fetch loop (#117) A code-review pass found the negotiation loop would POST once per bare `0000` flush that carried no haves, so a malformed `wants + N*0000 + EOF` stream amplified into N signed POSTs. Real git never sends a content-free mid-negotiation flush and the peer is upstream of the local git process, so this was not reachable in practice, but it left the loop unbounded per input. Skip a round that carries no new haves (an empty flush) and only POST rounds with have content or a terminator, which also makes the loop provably bounded by git's finite have set. The fresh-clone done-with-no-haves round still POSTs. --- crates/git-remote-gitlawb/src/main.rs | 52 ++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 5 deletions(-) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 49b3f9fe..9d8a7934 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -577,11 +577,16 @@ fn negotiate_upload_pack( loop { let (batch, bend) = read_upload_pack_round(stdin)?; - if batch.is_empty() && bend == RoundEnd::Eof { - // git closed with no new haves (clone with wants+flush+done is handled - // by the Done arm below; this is an up-to-date flush-only opener or a - // clean abort): nothing more to POST. - return Ok(()); + // Only POST when a round carries new haves or a terminator. A round with no + // new haves needs no request: an empty EOF ends the fetch, and a bare flush + // (git never sends a content-free flush mid-negotiation, but a malformed + // stream could) is skipped so a run of empty flushes cannot amplify into one + // signed POST each. A `done` round with no haves still POSTs (the fresh-clone + // case), handled by the terminator match below. + match (batch.is_empty(), bend) { + (true, RoundEnd::Eof) => return Ok(()), + (true, RoundEnd::Flush) => continue, + _ => {} } acc_haves.extend_from_slice(&batch); @@ -1918,4 +1923,41 @@ mod tests { "a single-shot negotiation must be exactly one POST, not split" ); } + + /// Hardening: content-free flush pkts between the wants and the first haves do + /// not each fire a POST. Real git never sends a bare mid-negotiation flush, but + /// a malformed stream must not amplify into one signed POST per empty flush; the + /// loop skips them and POSTs only the round that carries haves + done. + #[test] + fn repeated_bare_flushes_do_not_amplify_posts() { + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&want_line(&"a".repeat(40))); + stdin_bytes.extend_from_slice(b"0000"); + stdin_bytes.extend_from_slice(b"0000"); // bare flush, no haves + stdin_bytes.extend_from_slice(b"0000"); // bare flush, no haves + stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + stdin_bytes.extend_from_slice(&pkt("done\n")); + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("fetch with stray flushes should complete"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 2, + "content-free flushes must not each produce a POST" + ); + } } From 00e3f834021eb80cab775ae67618f2e5e60c5ac4 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 11 Jul 2026 20:25:26 -0500 Subject: [PATCH 4/4] test(git-remote): close branch-coverage gaps in the fetch loop (#117) Adds executed coverage for the negotiation branches the first pass left unrun: an invalid pkt-line length (rejected, not underflowed), a non-EOF read error (propagated, not swallowed as end-of-stream), an immediately empty upload-pack request (skips the POST), a nonempty have-batch terminated by EOF (POSTed as a final done round), and an empty receive-pack body (skips the POST). Each was mutation-checked to fail without its guard, so the coverage is load-bearing rather than vacuous. --- crates/git-remote-gitlawb/src/main.rs | 108 ++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 9d8a7934..e2593179 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -1960,4 +1960,112 @@ mod tests { "content-free flushes must not each produce a POST" ); } + + // ── Branch-coverage closure: exercise the remaining changed arms ───────── + + /// A reader that fails with a non-EOF io error, to exercise the error + /// propagation arm of `read_upload_pack_round` (distinct from a clean EOF). + struct FailingReader; + impl Read for FailingReader { + fn read(&mut self, _buf: &mut [u8]) -> io::Result { + Err(io::Error::other("simulated read failure")) + } + } + + /// G4: a non-EOF read error propagates; it is not swallowed as end-of-stream. + #[test] + fn read_upload_pack_round_propagates_read_error() { + let mut r = FailingReader; + assert!(read_upload_pack_round(&mut r).is_err()); + } + + /// G1: an invalid pkt-line length (1..=3) is rejected rather than underflowing + /// `pkt_len - 4`. + #[test] + fn read_upload_pack_round_rejects_invalid_pkt_length() { + let mut r = io::Cursor::new(b"0001".to_vec()); // pkt_len 1, < 4 + let err = read_upload_pack_round(&mut r).unwrap_err(); + assert!( + err.to_string().contains("invalid pkt-line length"), + "unexpected error: {err}" + ); + } + + /// G2: an immediately-empty upload-pack request (EOF at the opening round, with + /// a 200 advertisement) skips the POST entirely. + #[test] + fn upload_pack_empty_request_skips_post() { + let (base_url, server) = serve_http(vec![TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(Vec::::new()); // immediate EOF + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("empty upload-pack request should skip the POST"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 1, + "empty request must issue the GET only, no POST" + ); + } + + /// G3: a nonempty have-batch terminated by EOF (no flush, no done) is POSTed as + /// the final round, with a `done` terminator appended. + #[test] + fn nonempty_eof_batch_posts_as_final_round() { + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&want_line(&"a".repeat(40))); + stdin_bytes.extend_from_slice(b"0000"); + stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + // No trailing flush or done: the stream just ends (EOF) after the have. + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("nonempty EOF batch should POST as a final round"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 2, + "one POST for the EOF-terminated final round" + ); + assert!( + request_body(&requests[1]).ends_with(b"0009done\n"), + "an EOF-terminated final round is sent with a done terminator" + ); + } + + /// G5: an empty receive-pack (push) body skips the POST, unchanged from before. + #[test] + fn receive_pack_empty_body_skips_post() { + let (base_url, server) = serve_http(vec![TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-receive-pack", + status: "200 OK", + body: "0000", + }]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(Vec::::new()); // empty push + handle_connect(&repo_base, "git-receive-pack", None, &mut stdin) + .expect("empty receive-pack should skip the POST"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 1, + "empty push must issue the GET only, no POST" + ); + } }