diff --git a/crates/bashkit/src/builtins/cat.rs b/crates/bashkit/src/builtins/cat.rs index 5abefa942..620feb18c 100644 --- a/crates/bashkit/src/builtins/cat.rs +++ b/crates/bashkit/src/builtins/cat.rs @@ -77,7 +77,10 @@ impl Builtin for Cat { }; match ctx.fs.read_file(Path::new(&path)).await { Ok(bytes) => raw.extend_from_slice(&bytes), - Err(e) => return Ok(ExecResult::err(format!("cat: {file}: {e}\n"), 1)), + Err(e) => { + let reason = crate::error::io_error_reason(&e); + return Ok(ExecResult::err(format!("cat: {file}: {reason}\n"), 1)); + } } } } diff --git a/crates/bashkit/src/builtins/headtail.rs b/crates/bashkit/src/builtins/headtail.rs index 0850c7258..b46c17c17 100644 --- a/crates/bashkit/src/builtins/headtail.rs +++ b/crates/bashkit/src/builtins/headtail.rs @@ -81,7 +81,8 @@ impl Builtin for Head { } } Err(e) => { - return Ok(ExecResult::err(format!("head: {}: {}\n", file, e), 1)); + let reason = crate::error::io_error_reason(&e); + return Ok(ExecResult::err(format!("head: {file}: {reason}\n"), 1)); } } } diff --git a/crates/bashkit/src/builtins/mod.rs b/crates/bashkit/src/builtins/mod.rs index 4f9a6f64c..c27a1a79c 100644 --- a/crates/bashkit/src/builtins/mod.rs +++ b/crates/bashkit/src/builtins/mod.rs @@ -280,7 +280,18 @@ pub(crate) async fn read_stream_file( fs.read_file(path) .await .map(crate::StreamData::from) - .map_err(|e| ExecResult::err(format!("{cmd_name}: {}: {e}\n", path.display()), 1)) + .map_err(|e| { + // Never the `Display` of a `crate::Error`: `io error: ` is a Rust + // enum shape no tool prints. See `error::io_error_reason`. + ExecResult::err( + format!( + "{cmd_name}: {}: {}\n", + path.display(), + crate::error::io_error_reason(&e) + ), + 1, + ) + }) } /// Check args for `--help` and optionally `--version`. diff --git a/crates/bashkit/src/builtins/strings.rs b/crates/bashkit/src/builtins/strings.rs index 0245fa6f7..d0519ebdd 100644 --- a/crates/bashkit/src/builtins/strings.rs +++ b/crates/bashkit/src/builtins/strings.rs @@ -193,7 +193,8 @@ impl Builtin for Strings { output.push_str(&extract_strings(&content, &opts)); } Err(e) => { - return Ok(ExecResult::err(format!("strings: {}: {}\n", file, e), 1)); + let reason = crate::error::io_error_reason(&e); + return Ok(ExecResult::err(format!("strings: {file}: {reason}\n"), 1)); } } } diff --git a/crates/bashkit/tests/integration/builtin_fs_error_tests.rs b/crates/bashkit/tests/integration/builtin_fs_error_tests.rs new file mode 100644 index 000000000..d9494ac81 --- /dev/null +++ b/crates/bashkit/tests/integration/builtin_fs_error_tests.rs @@ -0,0 +1,97 @@ +//! Regression: a builtin that cannot read an operand must report the reason +//! the way the real tool does — `No such file or directory`, not the `Display` +//! of `bashkit::Error`, whose `io error: ` prefix is a Rust enum shape no tool +//! prints (TM-INF-022, sibling of the `internal error:` leak fixed for the hex +//! dumpers). +//! +//! Most of these tools share `builtins::read_text_file` / `read_stream_file`, +//! so one leaking formatter reached all of them at once; `cat`, `head` and +//! `strings` read the VFS directly and leaked it separately. +//! +//! Expectations taken from GNU coreutils 9.8. + +use bashkit::Bash; +use bashkit::testing::assert_no_leak; + +/// Every tool here takes a file operand and reads it through the VFS. +const READERS: &[(&str, &str)] = &[ + ("cat", "cat /nope"), + ("head", "head /nope"), + ("tail", "tail /nope"), + ("strings", "strings /nope"), + ("wc", "wc /nope"), + ("sort", "sort /nope"), + ("uniq", "uniq /nope"), + ("cut", "cut -c1 /nope"), + ("nl", "nl /nope"), + ("awk", "awk '{print}' /nope"), + ("paste", "paste /nope"), + ("rev", "rev /nope"), + ("tac", "tac /nope"), + ("column", "column /nope"), +]; + +#[tokio::test] +async fn missing_operand_reports_the_real_errno_text() { + for (name, script) in READERS { + let mut bash = Bash::new(); + let result = bash + .exec(script) + .await + .unwrap_or_else(|e| panic!("{script:?} aborted the script: {e}")); + let stderr = result.stderr.to_string(); + assert!( + stderr.contains("No such file or directory"), + "{name}: expected the real errno text, got {stderr:?}" + ); + assert_eq!(result.exit_code, 1, "{name} exit code"); + } +} + +/// The specific shape that regressed: the `Display` of `bashkit::Error`. +#[tokio::test] +async fn missing_operand_does_not_leak_the_error_enum_shape() { + for (name, script) in READERS { + let mut bash = Bash::new(); + let result = bash.exec(script).await.unwrap(); + let stderr = result.stderr.to_string(); + assert!( + !stderr.contains("io error:"), + "{name} leaks the Error enum shape: {stderr:?}" + ); + assert!( + !stderr.contains("internal error:"), + "{name} leaks the Error enum shape: {stderr:?}" + ); + // And the whole cross-tool banned list, so a reworded leak still fails. + assert_no_leak(&result, name, &[]); + } +} + +/// A backend reason richer than its errno is kept: collapsing it to the bare +/// errno would drop the only actionable detail. +#[tokio::test] +async fn read_only_backend_reason_survives() { + let mut bash = Bash::builder() + .fs(std::sync::Arc::new(bashkit::ReadOnlyFs::new( + std::sync::Arc::new(bashkit::InMemoryFs::new()), + ))) + .build(); + let result = bash.exec("echo hi > /nope.txt").await.unwrap(); + assert_eq!( + result.stderr.to_string(), + "bash: /nope.txt: filesystem is read-only\n" + ); +} + +/// A readable operand still works — the error path must not have swallowed +/// the success path. +#[tokio::test] +async fn readable_operand_still_reads() { + let mut bash = Bash::builder().mount_text("/ok.txt", "hello\n").build(); + for script in ["cat /ok.txt", "wc -l /ok.txt", "head /ok.txt"] { + let result = bash.exec(script).await.unwrap(); + assert_eq!(result.exit_code, 0, "{script} exit code"); + assert_eq!(result.stderr.to_string(), "", "{script} stderr"); + } +} diff --git a/crates/bashkit/tests/integration/main.rs b/crates/bashkit/tests/integration/main.rs index 162fa8b8b..aa4f16354 100644 --- a/crates/bashkit/tests/integration/main.rs +++ b/crates/bashkit/tests/integration/main.rs @@ -26,6 +26,7 @@ pub mod base64_binary_tests; pub mod bash_source_tests; pub mod blackbox_security_tests; pub mod builtin_error_security_tests; +pub mod builtin_fs_error_tests; pub mod builtin_registry_tests; pub mod byte_range_panic_tests; pub mod byte_stream_tests; diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index 9562314c9..1290cf5a5 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -466,6 +466,12 @@ reporting a filesystem failure must render the reason with custom `FileSystem` backend's own richer wording (`filesystem is read-only`) verbatim. Redirection diagnostics share the same helper. +Most file-reading builtins go through `builtins::read_text_file` / +`read_stream_file`, so a single leaking formatter there reached ~25 tools at +once (`wc`, `sort`, `cut`, `nl`, `awk`, `paste`, `uniq`, `rev`, `tac`, +`column`, `jq`, `diff`, …); `cat`, `head`, `tail` and `strings` read the VFS +directly and leaked it separately. Coverage: `builtin_fs_error_tests`. + A related shape: a user-reachable failure classified as `Error::Internal` aborts the whole script *and* prints `internal error:`. Nightly `glob_fuzz` run 228 caught `od`/`xxd`/`hexdump` doing this for a missing operand, where