diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 151d2738c..1a0004ca5 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -592,12 +592,17 @@ fn unavailable_command_hint(name: &str) -> Option<&'static str> { } /// Build a "command not found" error with optional suggestions. +/// +/// The message is newline-terminated: it goes straight into stderr, and real +/// bash ends every diagnostic with a newline. Without it consecutive failures +/// run together on one line (`...command not foundbash: next: ...`). fn command_not_found_message(name: &str, known_commands: &[&str]) -> String { let mut msg = format!("bash: {}: command not found", name); // Check for unavailable command hints first if let Some(hint) = unavailable_command_hint(name) { msg.push_str(&format!(". {}", hint)); + msg.push('\n'); return msg; } @@ -626,6 +631,7 @@ fn command_not_found_message(name: &str, known_commands: &[&str]) -> String { msg.push_str(&format!(". Did you mean: {}?", names.join(", "))); } + msg.push('\n'); msg } @@ -6531,7 +6537,7 @@ impl Interpreter { && matches!(name, "exec" | "bash" | "sh" | "source" | ".") { return Some(Ok(ExecResult::err( - format!("bash: {}: command not found", name), + format!("bash: {}: command not found\n", name), 127, ))); } @@ -6616,7 +6622,7 @@ impl Interpreter { if name.contains('/') { if self.shell_profile.is_logic_only() { return Ok(ExecResult::err( - format!("bash: {}: command not found", name), + format!("bash: {}: command not found\n", name), 127, )); } @@ -6716,7 +6722,7 @@ impl Interpreter { Ok(m) => m, Err(_) => { return Ok(ExecResult::err( - format!("bash: {}: No such file or directory", name), + format!("bash: {}: No such file or directory\n", name), 127, )); } @@ -6725,7 +6731,7 @@ impl Interpreter { // Directory check if meta.file_type.is_dir() { return Ok(ExecResult::err( - format!("bash: {}: Is a directory", name), + format!("bash: {}: Is a directory\n", name), 126, )); } @@ -6733,7 +6739,7 @@ impl Interpreter { // Execute permission check if meta.mode & 0o111 == 0 { return Ok(ExecResult::err( - format!("bash: {}: Permission denied", name), + format!("bash: {}: Permission denied\n", name), 126, )); } @@ -6743,7 +6749,7 @@ impl Interpreter { Ok(c) => decode_file_bytes_for_path(&path, &c), Err(_) => { return Ok(ExecResult::err( - format!("bash: {}: No such file or directory", name), + format!("bash: {}: No such file or directory\n", name), 127, )); } @@ -12176,7 +12182,7 @@ echo "count=$COUNT" let msg = command_not_found_message("grpe", &["type", "true", "tree", "grep"]); assert_eq!( msg, - "bash: grpe: command not found. Did you mean: grep, tree, true?" + "bash: grpe: command not found. Did you mean: grep, tree, true?\n" ); } diff --git a/crates/bashkit/src/interpreter/redirection.rs b/crates/bashkit/src/interpreter/redirection.rs index ac309b990..5d350a8ce 100644 --- a/crates/bashkit/src/interpreter/redirection.rs +++ b/crates/bashkit/src/interpreter/redirection.rs @@ -302,7 +302,9 @@ impl Interpreter { for redirect in redirects { if word_has_process_substitution(&redirect.target) { - return Some("bash: process substitution disabled in logic-only shell".to_string()); + return Some( + "bash: process substitution disabled in logic-only shell\n".to_string(), + ); } if matches!( diff --git a/crates/bashkit/tests/integration/glob_fuzz_scaffold_tests.rs b/crates/bashkit/tests/integration/glob_fuzz_scaffold_tests.rs index 9a0a67906..00a5f9925 100644 --- a/crates/bashkit/tests/integration/glob_fuzz_scaffold_tests.rs +++ b/crates/bashkit/tests/integration/glob_fuzz_scaffold_tests.rs @@ -200,3 +200,107 @@ async fn hex_dumpers_report_missing_operand_like_real_od() { ); } } + +/// Regression: fuzz run 229 (`crash-bd17afb20a1f27f7c30dc099a77b76e7b26f13bb`, +/// bytes `[10, 36, 114, 10, 126, 10, 45, 84, 111, 107, 58, 10, 126, 58]` +/// = `\n$r\n~\n-Tok:\n~:`). +/// +/// Three command-resolution diagnostics in a row. None of them terminated its +/// line, so all three arrived as a single run-on line +/// `bash: /home/sandbox: Is a directorybash: -Tok:: command not found...`. +/// The echo filter matches a diagnostic per line, and that line opened with a +/// recognized template but ended with a suggestion suffix, so nothing was +/// stripped and the `-Tok:` echo (rendered `-Tok::` by the `: ` separator) +/// tripped the banned parser-token shape `Tok::`. +#[tokio::test] +async fn glob_fuzz_crash_unterminated_command_diagnostics() { + let input = "\n$r\n~\n-Tok:\n~:"; + // The pre-filter cannot catch this one: the input holds `-Tok:` with a + // single colon, and the second colon comes from the template's own + // `: ` separator. The echo filter is the defense that has to hold. + assert!(!bashkit::testing::input_echo_would_trip(input)); + + let mut bash = fuzz_bash(); + for script in [ + format!("ls /tmp/{}", input), + format!( + "case \"test.txt\" in {}) echo match;; *) echo no;; esac", + input + ), + format!("if [[ \"hello.world\" == {} ]]; then echo y; fi", input), + ] { + fuzz_exec( + &mut bash, + &script, + "glob_fuzz_crash_unterminated_command_diagnostics", + &[], + ) + .await; + } +} + +/// Each command-resolution diagnostic terminates its own line, the way real +/// bash writes them. Verified against GNU bash 5.2.21: +/// +/// $ bash -c $'~\n-Tok:\nnotacmd' 2>&1 | cat -A +/// bash: line 1: /root: Is a directory$ +/// bash: line 2: -Tok:: command not found$ +/// bash: line 3: notacmd: command not found$ +#[tokio::test] +async fn consecutive_command_failures_are_separate_lines() { + let mut bash = fuzz_bash(); + let result = bash.exec("nosuchcmd\nalsomissing").await.unwrap(); + assert_eq!( + result.stderr.to_string(), + "bash: nosuchcmd: command not found\nbash: alsomissing: command not found\n" + ); +} + +/// Every `bash:` diagnostic the command resolver emits ends in a newline. +/// A run-on stderr is not just cosmetic: a host that splits stderr into +/// lines sees two failures as one, and the fuzz harness's echo filter — which +/// matches one real-shell template per line — stops recognizing them. +#[tokio::test] +async fn command_resolution_diagnostics_end_with_newline() { + let cases: &[(&str, &str)] = &[ + ("nosuchcmd", "bash: nosuchcmd: command not found\n"), + ("/tmp", "bash: /tmp: Is a directory\n"), + ( + "/nope/missing", + "bash: /nope/missing: No such file or directory\n", + ), + ]; + for (script, expected) in cases { + let mut bash = fuzz_bash(); + let result = bash.exec(script).await.unwrap(); + assert_eq!(result.stderr.to_string(), *expected, "script: {script}"); + } +} + +/// The suggestion variant (`. Did you mean: …`) is still one terminated line. +#[tokio::test] +async fn command_not_found_suggestion_ends_with_newline() { + let mut bash = fuzz_bash(); + let result = bash.exec("grpe").await.unwrap(); + let stderr = result.stderr.to_string(); + assert!( + stderr.starts_with("bash: grpe: command not found. Did you mean: "), + "unexpected wording: {stderr:?}" + ); + assert!(stderr.ends_with("\n"), "not terminated: {stderr:?}"); + assert_eq!(stderr.lines().count(), 1, "not one line: {stderr:?}"); +} + +/// A non-executable file reports `Permission denied` on its own line. +#[tokio::test] +async fn non_executable_script_denied_ends_with_newline() { + let mut bash = Bash::builder() + .mount_text("/tmp/noexec.sh", "echo hi") + .build(); + let result = bash.exec("/tmp/noexec.sh").await.unwrap(); + assert_eq!( + result.stderr.to_string(), + "bash: /tmp/noexec.sh: Permission denied\n" + ); + assert_eq!(result.exit_code, 126); +} diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index 9eb84f71a..5ec9b44c9 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -552,10 +552,35 @@ second, independent gap: cannot be swallowed *inside* a span because each shell diagnostic is formatted and written as one string with nothing interleaved. -Regression coverage: `tests/integration/glob_fuzz_scaffold_tests.rs` replays both -crash inputs through all three scripts the target builds, and the -`strip_*`/`input_echo_would_trip` unit tests in `src/testing.rs` pin both the -strip and keep directions. +Run 229 (`\n$r\n~\n-Tok:\n~:`) showed the assumption underneath the span rule was +only half true. The filter matches one diagnostic per line, which requires each +diagnostic to *end* its line — but the command-resolution diagnostics were built +with `format!("bash: {}: command not found", name)` and friends and never +terminated, so three consecutive failures arrived as one run-on line: + + bash: /home/sandbox: Is a directorybash: -Tok:: command not foundbash: ~:: command not found. Did you mean: :? + +That line opened with a recognized template but ended with a suggestion suffix, so +nothing was stripped and the echoed `-Tok:` — rendered `-Tok::` by the template's +own `: ` separator — tripped the parser-token shape `Tok::`. The pre-filter could +not have caught it either: the input holds a single colon, and the second one comes +from the template. + +Fixed at the source rather than in the filter. **Every `bash:` diagnostic written to +stderr terminates its own line**, which is what real bash does (`bash -c $'~\n-Tok:'` +prints two `$`-terminated lines under `cat -A`). The run-on was a user-visible defect +in its own right: a host that splits stderr into lines saw two failures as one. The +newline is added at the emit sites in `interpreter/mod.rs` (`command_not_found_message` +and the `try_execute_script_by_path` stat/dir/permission diagnostics) and at the +process-substitution refusal in `interpreter/redirection.rs`, whose sibling return in +the same function already terminated. `Error::Execution` payloads are deliberately +left unterminated: they surface through a `Display` impl, not through stderr. + +Regression coverage: `tests/integration/glob_fuzz_scaffold_tests.rs` replays all three +crash inputs through all three scripts the target builds, pins the terminator on each +command-resolution diagnostic, and asserts two consecutive failures land on separate +lines; the `strip_*`/`input_echo_would_trip` unit tests in `src/testing.rs` pin both +the strip and keep directions. **TM-INF-013**: The jq builtin previously called `std::env::set_var()` to expose shell variables to jaq's `env` function. This also made host process env vars (API keys, tokens)