Skip to main content

max / makenotwork

Close the five mutation survivors in git-command cargo mutants on astra, first run against the crate: 35 mutants, 27 caught, 3 unviable, 5 missed. astra-soak-overview's done condition is saturation AND zero survivors, so these are the other half of it. Four were plain test gaps. Hyphens and underscores are in the segment charset and nothing exercised them, so flipping an operator to refuse hyphens survived. ParseError's Display reaches a git client through anyhow and nothing asserted it said anything, let alone anything distinct. And the oracle itself was unobserved in both of its entry points: parse only ever yields requests the oracle accepts, so replacing either body with () passed every test. That last one is the failure this whole arrangement cannot afford, and it now has tests that build the bad requests parse will not -- a traversing owner, a separator in a segment, and a name that would break out of the git-shell quotes. check_line returns whether it checked, because otherwise nothing can observe it running at all. The fifth was not a test gap. The explicit pre-strip `..` test had become dead: valid_segment refuses `..` anywhere and a dot at either end, and those close the strip entirely -- if the raw remainder carries `..` and the stripped name does not, the `..` straddles the boundary, so the name ends with a dot and is refused. `.git` carries no `..` of its own. Every input the guard rejected was already rejected below, which is why no test could tell it from its own absence. Two overlapping guards is the same defect as two parsers, one level down, so it is gone and the ordering question with it. Verified by re-fuzzing after the removal rather than by the argument alone: 74,859,752 executions, no crashes.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-08-13 02:16 UTC
Signed with PGP, not checked
Commit: 451d72c9e5b7b0316f4d2f5031367051c545684b
Parent: cf64ad6
3 files changed, +132 insertions, -25 deletions
M shared/git-command/Cargo.lock +8 -8
@@ -7,12 +7,8 @@
7 7 version = "0.1.0"
8 8
9 9 [[patch.unused]]
10 - name = "synckit-client"
11 - version = "0.8.0"
12 -
13 - [[patch.unused]]
14 - name = "synckit-config"
15 - version = "0.2.0"
10 + name = "docengine"
11 + version = "0.7.0"
16 12
17 13 [[patch.unused]]
18 14 name = "kberg"
@@ -55,5 +51,9 @@
55 51 version = "0.1.0"
56 52
57 53 [[patch.unused]]
58 - name = "docengine"
59 - version = "0.7.0"
54 + name = "synckit-client"
55 + version = "0.8.0"
56 +
57 + [[patch.unused]]
58 + name = "synckit-config"
59 + version = "0.2.0"
M shared/git-command/src/lib.rs +119 -17
@@ -205,12 +205,24 @@
205 205 /// `mnw-cli`'s rule and the stricter one.
206 206 /// 3. **At most one leading slash is stripped.** The server stripped every
207 207 /// leading slash, so `//max/shop.git` parsed. Git sends at most one.
208 - /// 4. **`..` is tested before `.git` is stripped.** This is the one place the
209 - /// server's order wins, and it is the only class of the four that had teeth:
210 - /// `mnw-cli` stripped first, so `/max/shop..git` became repo `shop.` and was
211 - /// accepted. It was safe only because the charset guard downstream happened
212 - /// to catch what was left, which is a guard doing another guard's job. Here
213 - /// the raw segment is checked for `..` while it is still raw.
208 + /// 4. **`..` cannot survive the `.git` strip.** The two parsers tested for `..`
209 + /// on opposite sides of the suffix strip, and this was the only class of the
210 + /// four with teeth: `mnw-cli` stripped first, so `/max/shop..git` became repo
211 + /// `shop.` and was accepted, safe only because a charset guard further in
212 + /// happened to catch what was left.
213 + ///
214 + /// There is no ordering here to get wrong, because there is one check and it
215 + /// runs last. [`valid_segment`] refuses `..` anywhere and a dot at either
216 + /// end, and those two rules together close the strip entirely: if the raw
217 + /// remainder contains `..` and the stripped name does not, the `..` must
218 + /// straddle the boundary, so the remainder ends `..git` and the stripped name
219 + /// ends with a dot. `.git` carries no `..` of its own, so there is no other
220 + /// way across.
221 + ///
222 + /// An explicit pre-strip test stood here until `cargo mutants` reported it as
223 + /// a survivor on 2026-08-12. It was: every input it rejected was already
224 + /// rejected below, so no test could tell it from its own absence. Two
225 + /// overlapping guards is the same defect as two parsers, one level down.
214 226 pub fn parse(command_line: &str) -> Result<Request<'_>, ParseError> {
215 227 let (verb, rest) = command_line.split_once(' ').ok_or(ParseError::NoArgument)?;
216 228 let operation = Operation::from_wire(verb).ok_or(ParseError::UnsupportedOperation)?;
@@ -224,16 +236,11 @@
224 236
225 237 let (owner, rest) = path.split_once('/').ok_or(ParseError::MissingOwner)?;
226 238
227 - // (4) The traversal test runs on the raw remainder, before any suffix comes
228 - // off. `rest` may still contain `/` at this point; that is intentional, and
229 - // the `valid_segment` call below is what rejects it. Testing `..` here as a
230 - // substring covers every segment of a multi-segment remainder at once.
231 - if owner.contains("..") || rest.contains("..") {
232 - return Err(ParseError::InvalidSegment);
233 - }
234 -
239 + // `rest` may still contain `/` here; `valid_segment` is what refuses it, so
240 + // a nested path never becomes a repo name.
235 241 let repo = rest.strip_suffix(".git").unwrap_or(rest);
236 242
243 + // (4) The single gate. Both segments, after the strip.
237 244 if !valid_segment(owner) || !valid_segment(repo) {
238 245 return Err(ParseError::InvalidSegment);
239 246 }
@@ -358,9 +365,21 @@
358 365 ///
359 366 /// The entry point both the fuzz target and the regression replay call, so
360 367 /// "what the fuzzer checks" has exactly one definition.
361 - pub fn check_line(line: &str) {
362 - if let Ok(request) = super::parse(line) {
363 - check(&request);
368 + ///
369 + /// Returns whether a request was actually checked. The fuzz target ignores
370 + /// that and should: a line the grammar refuses is a fine thing to feed it.
371 + /// It exists because without a return value nothing can observe this
372 + /// function running at all — `cargo mutants` replaced the body with `()` on
373 + /// 2026-08-12 and every test still passed, since `parse` only ever yields
374 + /// requests the oracle accepts. A silently empty oracle is the one failure
375 + /// this whole arrangement cannot afford.
376 + pub fn check_line(line: &str) -> bool {
377 + match super::parse(line) {
378 + Ok(request) => {
379 + check(&request);
380 + true
381 + }
382 + Err(_) => false,
364 383 }
365 384 }
366 385 }
@@ -391,6 +410,44 @@
391 410 );
392 411 }
393 412
413 + /// Hyphens and underscores are in the charset and were untested until
414 + /// `cargo mutants` pointed it out on 2026-08-12: flipping an operator in
415 + /// `valid_segment` so hyphens were refused survived every test here.
416 + #[test]
417 + fn the_full_charset_is_accepted() {
418 + let r = ok("git-upload-pack my_user-1/my-repo_v2.0.git");
419 + assert_eq!((r.owner, r.repo), ("my_user-1", "my-repo_v2.0"));
420 + }
421 +
422 + #[test]
423 + fn every_parse_error_says_which_one() {
424 + // The `Display` impl reaches a git client through `anyhow`, so an
425 + // impl that returned an empty string would degrade every refusal on
426 + // the sshd door into a blank message.
427 + for (line, want) in [
428 + ("git-upload-pack", ParseError::NoArgument),
429 + ("git-foo /max/shop.git", ParseError::UnsupportedOperation),
430 + ("git-upload-pack shop.git", ParseError::MissingOwner),
431 + ("git-upload-pack /max/../etc", ParseError::InvalidSegment),
432 + ] {
433 + let err = parse(line).expect_err("should refuse");
434 + assert_eq!(err, want, "{line}");
435 + assert!(!err.to_string().is_empty(), "{want:?} displays as nothing");
436 + }
437 + // And the four are distinguishable, not one message four times.
438 + let shown = [
439 + ParseError::NoArgument,
440 + ParseError::UnsupportedOperation,
441 + ParseError::MissingOwner,
442 + ParseError::InvalidSegment,
443 + ]
444 + .map(|e| e.to_string());
445 + let mut uniq = shown.to_vec();
446 + uniq.sort();
447 + uniq.dedup();
448 + assert_eq!(uniq.len(), 4, "ParseError messages collide: {shown:?}");
449 + }
450 +
394 451 #[test]
395 452 fn quoting_and_slashes_a_client_may_send() {
396 453 for cmd in [
@@ -564,4 +621,49 @@
564 621 let r = ok("git-upload-pack \"max/shop\"");
565 622 assert_eq!(r.shell_command(), "git-upload-pack '/max/shop.git'");
566 623 }
624 +
625 + // ── The oracle, which nothing else can prove is running ──
626 + //
627 + // `parse` only ever yields requests the oracle accepts, so every test above
628 + // passes just as well against an oracle whose body is `()`. `cargo mutants`
629 + // made exactly that substitution on 2026-08-12 and nothing noticed. These
630 + // build the bad requests `parse` will not, and assert the oracle rejects
631 + // them.
632 +
633 + fn req(owner: &'static str, repo: &'static str) -> Request<'static> {
634 + Request {
635 + operation: Operation::UploadPack,
636 + owner,
637 + repo,
638 + }
639 + }
640 +
641 + #[test]
642 + fn the_oracle_accepts_a_good_request() {
643 + oracle::check(&req("max", "shop"));
644 + }
645 +
646 + #[test]
647 + fn the_oracle_rejects_a_traversing_owner() {
648 + assert!(std::panic::catch_unwind(|| oracle::check(&req("..", "shop"))).is_err());
649 + }
650 +
651 + #[test]
652 + fn the_oracle_rejects_a_separator_in_a_segment() {
653 + // The shape that would put a second path component under the root.
654 + assert!(std::panic::catch_unwind(|| oracle::check(&req("max", "a/b"))).is_err());
655 + }
656 +
657 + #[test]
658 + fn the_oracle_rejects_a_name_that_would_break_out_of_the_quotes() {
659 + // The injection shape. `shell_command` would emit
660 + // `git-upload-pack '/max/x'; rm -rf /.git'`, which is two words.
661 + assert!(std::panic::catch_unwind(|| oracle::check(&req("max", "x'; rm -rf /"))).is_err());
662 + }
663 +
664 + #[test]
665 + fn check_line_reports_whether_it_checked() {
666 + assert!(oracle::check_line("git-upload-pack /max/shop.git"));
667 + assert!(!oracle::check_line("not a git command"));
668 + }
567 669 }
M shared/git-command/fuzz/seeds/README.md +5
@@ -16,8 +16,13 @@
16 16
17 17 Run against these on a machine with no corpus:
18 18
19 + mkdir -p fuzz/corpus/command
19 20 cargo +nightly fuzz run command fuzz/corpus/command fuzz/seeds/command
20 21
22 + The `mkdir` is needed once. `cargo fuzz` creates the default corpus directory
23 + for you only when you name no directories at all; pass them explicitly and
24 + libFuzzer requires every one to exist already.
25 +
21 26 **Name the corpus directory first and this one second.** libFuzzer writes new
22 27 inputs into whichever directory it is given first and treats the rest as
23 28 read-only. Passing `fuzz/seeds/command` alone dumps hundreds of machine-generated