From 948ddd9cefae2f4f5b1031a28338c7e5cabed2d3 Mon Sep 17 00:00:00 2001 From: Adam Poulemanos Date: Mon, 29 Jun 2026 22:44:54 -0400 Subject: [PATCH] test(p2): make cannot-fail error tests meaningful + un-fake null-byte (#62 P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #62 audit flagged several tests that pass without proving anything. This strengthens the clearly-tractable ones and removes a fabricated harness result: - TestHarness::run_submod no longer intercepts NUL-byte args and fabricates a failure. std's Command rejects interior NULs before spawn, so the real behavior is an Err at the process boundary — tests now assert that. - test_invalid_config_file_path had zero assertions. Now asserts a missing --config is handled gracefully (exit 0) AND a malformed config fails non-zero with a specific "parse error" diagnostic (not swallowed). - test_invalid_sparse_checkout_patterns asserted only !stderr.is_empty(). Now asserts path-traversal CONTAINMENT (a sentinel never appears outside the working tree, nor at an absolute location) for ../ patterns, and asserts the real NUL-byte boundary rejection instead of relying on the harness fake. - test_sparse_checkout_empty_patterns dropped its "failure also acceptable" escape; now requires success and that no non-empty sparse checkout is enabled. Behavior probed against the real binary first so the assertions are non-vacuous. Full suite 555 pass; fmt + clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01T8D5ZK1473YCiZkbueAY2X --- tests/common/mod.rs | 17 ++----- tests/error_handling_tests.rs | 92 +++++++++++++++++++++++++++------- tests/sparse_checkout_tests.rs | 54 +++++++++++--------- 3 files changed, 107 insertions(+), 56 deletions(-) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index fc1acfd..df582ae 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -5,7 +5,6 @@ //! Common utilities for integration tests use std::fs; -use std::os::unix::process::ExitStatusExt; use std::path::PathBuf; use std::process::Command; use tempfile::TempDir; @@ -255,18 +254,10 @@ impl TestHarness { &self, args: &[&str], ) -> Result> { - // Check for null bytes in arguments which would cause process execution to fail - for arg in args { - if arg.contains('\0') { - // Return a simulated failed output for null byte arguments - return Ok(std::process::Output { - status: std::process::ExitStatus::from_raw(1), - stdout: Vec::new(), - stderr: b"Error: Invalid argument contains null byte\n".to_vec(), - }); - } - } - + // NOTE: arguments containing an interior NUL byte cannot be passed to a + // process at all — std's Command rejects them before spawn, so `.output()` + // below returns an Err. We deliberately do NOT fabricate a fake failure + // here; tests assert the real process-boundary rejection. let output = Command::new(&self.submod_bin) .args(args) .current_dir(&self.work_dir) diff --git a/tests/error_handling_tests.rs b/tests/error_handling_tests.rs index ecc7bcd..aeb8378 100644 --- a/tests/error_handling_tests.rs +++ b/tests/error_handling_tests.rs @@ -69,13 +69,34 @@ mod tests { let harness = TestHarness::new().expect("Failed to create test harness"); harness.init_git_repo().expect("Failed to init git repo"); - // Try to use a non-existent config file - harness + // A non-existent config path is handled gracefully: `check` falls back to + // defaults and exits 0 (there are no submodules to report on). + let missing = harness .run_submod(&["--config", "/nonexistent/path/config.toml", "check"]) .expect("Failed to run submod"); + assert!( + missing.status.success(), + "a missing config file should be handled gracefully, exit was {:?}; stderr: {}", + missing.status.code(), + String::from_utf8_lossy(&missing.stderr) + ); - // Should handle missing config file gracefully (create default or error) - // The exact behavior depends on implementation + // A config file that exists but is malformed must NOT be swallowed: it + // fails with a specific parse diagnostic, not a generic catch-all. + let bad_config = harness.work_dir.join("bad.toml"); + fs::write(&bad_config, "this is = = not toml [[[\n").expect("write bad config"); + let malformed = harness + .run_submod(&["--config", "bad.toml", "check"]) + .expect("Failed to run submod"); + assert!( + !malformed.status.success(), + "a malformed config must cause a non-zero exit" + ); + let stderr = String::from_utf8_lossy(&malformed.stderr); + assert!( + stderr.contains("TOML parse error") || stderr.contains("parse error"), + "malformed config error must name the parse failure, got: {stderr}" + ); } #[test] @@ -202,16 +223,25 @@ mod tests { .expect("Failed to create remote"); let remote_url = format!("file://{}", remote_repo.display()); - // Test various potentially problematic sparse patterns - let problematic_patterns = vec![ - "//invalid//path", - "..", - "../../../escape", - "path/with/\0/null", - ]; + // A path-traversal sentinel: if any pattern lets the tool write outside the + // superproject working tree, this file/dir would appear one level up. + let escape_sentinel = harness + .work_dir + .parent() + .expect("work_dir has a parent") + .join("escape"); + assert!( + !escape_sentinel.exists(), + "sentinel must not pre-exist before the test" + ); - for pattern in problematic_patterns { - let output = harness + // Traversal / malformed patterns. The security-relevant property is + // CONTAINMENT: regardless of success or failure, nothing is written + // outside the superproject working tree. + let traversal_patterns = vec!["//invalid//path", "..", "../../../escape", "../escape"]; + + for pattern in traversal_patterns { + let _output = harness .run_submod(&[ "add", &remote_url, @@ -224,15 +254,41 @@ mod tests { ]) .expect("Failed to run submod"); - // Should either succeed and handle the pattern safely, or fail gracefully - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - assert!(!stderr.is_empty()); - } + // Containment: the traversal pattern must never materialize a path + // outside the working tree, nor at an absolute system location. + assert!( + !escape_sentinel.exists(), + "path traversal escaped the working tree for pattern {pattern:?}: {} was created", + escape_sentinel.display() + ); + assert!( + !std::path::Path::new("/escape").exists(), + "path traversal reached an absolute location for pattern {pattern:?}" + ); // Clean up for next iteration let _ = fs::remove_dir_all(harness.work_dir.join("lib/sparse-test")); } + + // A NUL byte in an argument cannot be handed to a process at all: std's + // Command rejects it before spawn, so run_submod returns an Err. Assert + // that real boundary rejection (previously faked by the harness). + let nul_result = harness.run_submod(&[ + "add", + &remote_url, + "--name", + "sparse-test", + "--path", + "lib/sparse-test", + "--sparse-paths", + "path/with/\0/null", + ]); + let err = + nul_result.expect_err("a NUL-byte argument must be rejected at the process boundary"); + assert!( + err.to_string().to_lowercase().contains("nul"), + "expected a NUL-byte rejection error, got: {err}" + ); } #[test] diff --git a/tests/sparse_checkout_tests.rs b/tests/sparse_checkout_tests.rs index f646505..1ad38c6 100644 --- a/tests/sparse_checkout_tests.rs +++ b/tests/sparse_checkout_tests.rs @@ -327,31 +327,35 @@ sparse_paths = ["src", "docs", "*.md"] .expect("Failed to create remote"); let remote_url = format!("file://{}", remote_repo.display()); - // Try to add with empty sparse paths - should handle gracefully - let output = harness.run_submod(&[ - "add", - &remote_url, - "--name", - "sparse-empty", - "--path", - "lib/sparse-empty", - "--sparse-paths", - "", - ]); - - // Should either succeed without sparse checkout or provide clear error - if let Ok(process_output) = output { - if process_output.status.success() { - // If successful, sparse checkout should not be enabled - let sparse_file = harness.get_sparse_checkout_file_path("lib/sparse-empty"); - assert!( - !sparse_file.exists() - || fs::read_to_string(&sparse_file).unwrap().trim().is_empty() - ); - } - } else { - // If it fails, that's also acceptable for empty patterns - } + // Adding with an empty sparse-paths value succeeds and simply does not + // enable sparse checkout (an empty pattern set is a no-op, not an error). + let output = harness + .run_submod(&[ + "add", + &remote_url, + "--name", + "sparse-empty", + "--path", + "lib/sparse-empty", + "--sparse-paths", + "", + ]) + .expect("Failed to run submod"); + + assert!( + output.status.success(), + "empty sparse-paths should be a graceful no-op, exit was {:?}; stderr: {}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + + // Sparse checkout must not be enabled: either no sparse-checkout file, or + // an empty one. + let sparse_file = harness.get_sparse_checkout_file_path("lib/sparse-empty"); + assert!( + !sparse_file.exists() || fs::read_to_string(&sparse_file).unwrap().trim().is_empty(), + "empty sparse-paths must not enable a non-empty sparse checkout" + ); } #[test]