fix: prevented shell command corruption in sequential chains

- Validated every stage of a pipeline against `simple_command_is_safe` instead of only the first stage to prevent improper segmentation of compound shell constructs.
- Guarded segment re-execution by verifying that each `Display`-reconstructed command parses back to the expected pipeline shape.
- Configured segmented-chain execution to fall back to an unsegmented, whole-command path whenever a reconstructed segment diverges from the original AST.
- Resolved a syntax error during command execution by preventing `Display` from stripping terminators from compound commands like `while` and `for` loops.
This commit is contained in:
can1357
2026-06-19 02:39:44 +02:00
parent 623ae8df35
commit 2e6e56711f
4 changed files with 221 additions and 48 deletions
+155 -48
View File
@@ -28,7 +28,7 @@ use brush_parser::{
ParserOptions, SourceInfo,
ast::{
AndOr, Command, CommandPrefixOrSuffixItem, CompoundListItem, IoFileRedirectTarget,
IoRedirect, Pipeline, Program, SeparatorOperator, Word,
IoRedirect, Pipeline, Program, SeparatorOperator, SimpleCommand, Word,
},
};
@@ -66,21 +66,24 @@ pub enum CommandPlan {
/// Parse `command` with `brush-parser` and classify its structure.
#[must_use]
pub fn analyze(command: &str) -> CommandPlan {
let trimmed = command.trim();
if trimmed.is_empty() {
if command.trim().is_empty() {
return CommandPlan::Unsupported;
}
let Some(program) = parse(command) else {
return CommandPlan::Unsupported;
};
classify(&program)
}
/// Parse `command` with the same `brush-parser` configuration the vendored
/// runtime uses, discarding error detail. Returns `None` on any syntax error
/// or unsupported construct.
fn parse(command: &str) -> Option<Program> {
let options = ParserOptions::default();
let source_info = SourceInfo::default();
let reader = std::io::Cursor::new(command.as_bytes());
let mut parser = brush_parser::Parser::new(reader, &options, &source_info);
let Ok(program) = parser.parse_program() else {
return CommandPlan::Unsupported;
};
classify(&program)
parser.parse_program().ok()
}
fn classify(program: &Program) -> CommandPlan {
@@ -216,51 +219,109 @@ fn io_redirect_is_safe(io: &IoRedirect) -> bool {
}
}
/// True when every part of a simple command is safe to reconstruct through
/// `brush`'s `Display` impl: no command/process substitutions and no here-doc
/// in the command word, prefix, or suffix (those re-emit in forms that diverge
/// from the source). The full reconstruction is still re-parse-verified by
/// [`reconstruction_reparses_to_same_shape`] before any segment is executed.
fn simple_command_is_safe(simple: &SimpleCommand) -> bool {
if let Some(prefix) = simple.prefix.as_ref()
&& prefix.0.iter().any(|item| !command_prefix_or_suffix_item_is_safe(item))
{
return false;
}
if let Some(suffix) = simple.suffix.as_ref()
&& suffix.0.iter().any(|item| !command_prefix_or_suffix_item_is_safe(item))
{
return false;
}
if let Some(word) = simple.word_or_name.as_ref()
&& word_has_command_substitution(word)
{
return false;
}
true
}
fn simple_segment(pipeline: &Pipeline) -> Option<(String, String)> {
if pipeline.timed.is_some() || pipeline.bang || pipeline.seq.is_empty() {
return None;
}
// For multi-stage pipes inside a chain segment, identify the segment by its
// first stage's program. The downstream per-segment minimizer::apply will
// detect the pipeline at runtime via plan::CommandPlan::Piped and pass it
// through unchanged — so a piped segment is safely captured but never
// rewritten. This keeps the chain decomposable when even one inner stage
// uses a pipe (e.g. `ls | head -10 && git status`).
let first = pipeline.seq.first()?;
match first {
Command::Simple(simple) => {
if simple.prefix.as_ref().is_some_and(|prefix| {
prefix
.0
.iter()
.any(|item| !command_prefix_or_suffix_item_is_safe(item))
}) {
return None;
}
if simple.suffix.as_ref().is_some_and(|suffix| {
suffix
.0
.iter()
.any(|item| !command_prefix_or_suffix_item_is_safe(item))
}) {
return None;
}
let program_word = simple.word_or_name.as_ref()?;
if word_has_command_substitution(program_word) {
return None;
}
let program = program_word.to_string();
if program.trim().is_empty() {
return None;
}
Some((pipeline.to_string(), program))
},
// Compound shell syntax (if / for / while / subshell / { ... }) is
// not something the minimizer should touch.
Command::Compound(..) | Command::Function(_) | Command::ExtendedTest(..) => None,
// Every stage must be a Display-safe simple command. Compound stages
// (`if` / `for` / `while` / subshells / `{ … }`) and unsafe words/redirects
// (here-docs, substitutions) do not round-trip through `Display`. Validating
// only `seq.first()` once let a compound later stage through — e.g.
// `git log … | while read x; do … done` — and the reconstructed segment then
// failed to execute with "syntax error at end of input".
for command in &pipeline.seq {
let Command::Simple(simple) = command else {
return None;
};
if !simple_command_is_safe(simple) {
return None;
}
}
// Identify the segment by its first stage's program word. Multi-stage pipes
// are captured but never rewritten (runtime detects `CommandPlan::Piped`),
// keeping the chain decomposable when an inner stage pipes (e.g.
// `ls | head -10 && git status`).
let Command::Simple(first) = pipeline.seq.first()? else {
return None;
};
let program_word = first.word_or_name.as_ref()?;
let program = program_word.to_string();
if program.trim().is_empty() {
return None;
}
// The chain runner re-executes this reconstructed string verbatim. brush's
// `Display` is not a guaranteed inverse of its parser, so re-parse the
// reconstruction and require the same pipeline shape before committing to
// segmentation. Any divergence (a lossy compound terminator today, a future
// `Display` change tomorrow) falls back to the unsegmented whole-command
// path instead of failing at execution.
let command = pipeline.to_string();
if !reconstruction_reparses_to_same_shape(&command, pipeline.seq.len()) {
return None;
}
Some((command, program))
}
/// Confirm a reconstructed segment string re-parses to the *same shape* it was
/// built from: exactly one sequential top-level pipeline with `expected_stages`
/// commands, no `&&`/`||`/`;`/`&` continuation, and no `!`/`time` modifier.
///
/// This is a syntax/shape guard, not a proof of full semantic equivalence. It
/// guarantees the chain runner never executes a reconstruction that fails to
/// parse or that `Display` reshaped into different top-level structure. brush's
/// `Display` is not a guaranteed inverse of its parser — compound terminators
/// (`while … done`) and quoted here-doc close tags re-emit in forms that fail
/// to re-parse — so a divergent segment drops back to the unsegmented
/// whole-command path instead of blowing up at execution with
/// "pi-natives:command: syntax error". The per-stage `simple_command_is_safe`
/// whitelist already excludes constructs whose `Display` is value-lossy
/// (substitutions, here-docs); words carry raw source text and round-trip
/// verbatim.
fn reconstruction_reparses_to_same_shape(reconstructed: &str, expected_stages: usize) -> bool {
let Some(program) = parse(reconstructed) else {
return false;
};
let mut items = program.complete_commands.iter().flat_map(|cl| cl.0.iter());
let Some(CompoundListItem(and_or, separator)) = items.next() else {
return false;
};
// A second top-level item, a trailing `&` (Async), or an `&&`/`||`
// continuation all mean `Display` reshaped the command.
if items.next().is_some()
|| !and_or.additional.is_empty()
|| matches!(separator, SeparatorOperator::Async)
{
return false;
}
let pipeline = &and_or.first;
!pipeline.bang && pipeline.timed.is_none() && pipeline.seq.len() == expected_stages
}
fn classify_pipeline(pipeline: &Pipeline) -> Option<CommandPlan> {
@@ -453,4 +514,50 @@ mod tests {
assert_eq!(analyze(""), CommandPlan::Unsupported);
assert_eq!(analyze(" "), CommandPlan::Unsupported);
}
#[test]
fn compound_later_pipeline_stage_is_not_segmented() {
// Regression: a pipeline whose *later* stage is a compound command
// (`while`/`for`/`if`/subshell) must never be segmented. The chain runner
// re-executes `pipeline.to_string()`, and brush's `Display` drops the
// compound terminator, so the reconstruction failed to re-parse and blew
// up at execution with "syntax error at end of input". Validating only
// `seq.first()` (a simple `git`/`seq`) let it slip through.
assert_not_chain(
"echo start && git log --oneline | while read h; do echo \"$h\"; done | head -3",
);
assert_not_chain("seq 3 | while read n; do echo \"$n\"; done && echo done");
assert_not_chain("printf x && ls | for f in a b; do echo $f; done");
// The reported shape classifies as Compound, so it runs whole and
// unsegmented via the single path (no reconstruction, no minimization).
assert_eq!(
analyze("echo a && seq 2 | while read n; do echo $n; done"),
CommandPlan::Compound,
);
}
#[test]
fn chain_segments_reparse_cleanly() {
// Every command string the chain runner will execute must itself re-parse
// to a single pipeline — the contract enforced by
// `reconstruction_reparses_to_same_shape`. A segment whose `Display`
// reconstruction diverges is dropped rather than emitted.
for command in [
"git diff --stat && git diff --name-only",
"ls -lh *.txt | head -5 && git status --short",
"grep -c foo bar 2>/dev/null && echo done",
"FOO=1 git status ; bun test",
] {
let Some(segments) = chain_of(analyze(command)) else {
panic!("{command:?} should classify as Chain");
};
for segment in &segments {
assert!(
parse(&segment.command).is_some(),
"segment {:?} from {command:?} did not re-parse",
segment.command,
);
}
}
}
}
+58
View File
@@ -2237,6 +2237,64 @@ replace = [{ pattern = "^.+$", replacement = "PWD" }]
assert!(!output.contains("unterminated"));
}
/// Regression: a `&&` / `;` chain whose later pipeline stage is a compound
/// command (`while … done`) must execute instead of failing with
/// "pi-natives:command: syntax error at end of input". The segmented chain
/// runner rebuilt each segment via the brush AST `Display` impl, but only
/// validated the *first* pipeline stage — so a compound later stage was
/// reconstructed without its terminator and re-run as invalid shell. Such a
/// command now bails out of segmentation and runs whole via the single path.
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread")]
async fn compound_stage_in_chain_runs_via_single_path() {
let root = unique_temp_dir("compound-chain");
let minimizer = printf_minimizer(&root.join("minimizer.toml"), None);
let (result, output) = run_command_capture(
"printf 'start\\n' && seq 5 | while read n; do echo \"n=$n\"; done | head -2",
None,
Some(minimizer),
CancelToken::default(),
)
.await;
let _ = std::fs::remove_dir_all(&root);
assert_eq!(result.exit_code, Some(0));
assert_eq!(output, "start\nn=1\nn=2\n");
assert!(!output.contains("syntax error"));
// Ran whole (unsegmented), so nothing was minimized.
assert!(result.minimized.is_none());
}
/// A segment that carries a file redirect is still segmented, and the brush
/// `Display` reconstruction the runner executes must round-trip through
/// brush's own parser **without losing the redirect**. `echo hidden >/dev/null`
/// suppresses its own stdout: if the reconstruction dropped the redirect,
/// `hidden` would leak into the captured output. Proves the reconstruction
/// path is semantically sound for the redirect-bearing shapes the per-stage
/// whitelist accepts (not just syntactically parseable).
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread")]
async fn segmented_chain_with_redirect_executes_correctly() {
let root = unique_temp_dir("redirect-chain");
let minimizer = printf_minimizer(&root.join("minimizer.toml"), None);
let (result, output) = run_command_capture(
"echo hidden >/dev/null && printf 'hello\\n'",
None,
Some(minimizer),
CancelToken::default(),
)
.await;
let _ = std::fs::remove_dir_all(&root);
assert_eq!(result.exit_code, Some(0));
// The redirect survived reconstruction: segment 1's stdout went to
// /dev/null, so only segment 2's output is captured.
assert!(!output.contains("hidden"), "redirect must suppress segment-1 stdout");
assert_eq!(output, "hello\n");
let minimized = result.minimized.expect("redirect chain should be minimized");
assert_eq!(minimized.original_text, "hello\n");
assert_eq!(minimized.text, "HI\n");
assert!(!output.contains("syntax error"));
}
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread")]
async fn segmented_chain_exceeding_aggregate_capture_cap_stays_raw() {
+4
View File
@@ -2,6 +2,10 @@
## [Unreleased]
### Fixed
- Fixed the bash tool failing with `pi-natives:command: syntax error at end of input` on a valid `&&`/`;` chain whose later pipeline stage is a compound command, e.g. `echo x && git log | while read h; do …; done | head`. The minimizer's segmented-chain runner rebuilds each chain segment from the brush AST via `pipeline.to_string()` and re-executes that string, but `simple_segment` only validated the *first* pipeline stage — so a compound later stage (`while`/`for`/`if`/subshell) was re-serialized without its terminator and re-run as broken shell. Every stage is now required to be a Display-safe simple command, and — as a general guard against the recurring class of brush `Display` round-trip divergences (previously: quoted here-doc close tags, multi-byte char/byte offsets) — each reconstructed segment is now re-parsed and must match the original pipeline shape before the chain runner executes it; any divergence runs the command whole, unsegmented, instead of corrupting it.
## [16.0.10] - 2026-06-18
### Added
+4
View File
@@ -2,6 +2,10 @@
## [Unreleased]
### Fixed
- Fixed native shell execution reporting `pi-natives:command: syntax error at end of input` for a valid `&&`/`;` chain whose later pipeline stage is a compound command, e.g. `echo x && git log | while read h; do …; done | head`. The output minimizer's segmented-chain runner rebuilds each chain segment from the brush-parser AST via `pipeline.to_string()` and re-executes that string, but `simple_segment` only validated the *first* pipeline stage — so a compound later stage (`while`/`for`/`if`/subshell) was re-serialized without its terminator (`Display` drops it) and re-run as broken shell. `simple_segment` now requires every stage to be a `Display`-safe simple command, and — closing the recurring class of brush `Display` round-trip divergences (here-doc close-tag quoting, multi-byte char/byte offsets) at its root — each reconstructed segment is re-parsed and must match the original pipeline shape before the chain runner executes it; any divergence runs the command whole via the unsegmented path instead of corrupting it.
## [16.0.7] - 2026-06-18
### Added