From d7fe6f1a2f32be80682d0e9292fbfb2c78d0ef49 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Sat, 25 Jul 2026 19:00:58 +0200 Subject: [PATCH 1/2] fix(config): Keep template spans intact in command strings Command strings written as plain strings (rather than the structured `program`/`args` form) are split into shell words before minijinja renders any `{{ }}`, `{% %}`, or `{# #}` spans. Until now that split used `shlex::split` directly, so a template expression containing spaces was torn across multiple arguments, e.g. `just x {{ a | default('') }}` produced `args: ["x", "{{", "a", "|", "default('')", "}}"]` instead of a single argument. Template spans are now masked with NUL-delimited placeholders before the shell split and restored afterwards, so a span survives as one argument even when it contains spaces or quoted filter arguments. String literals inside expression/statement spans are tracked so a `}}`/`%}` inside a quoted string does not end the span early, and an unterminated span is kept whole rather than mangled, leaving the error to minijinja at render time. Quotes inside a span (e.g. inside a `{# comment #}`) no longer trip the unbalanced-shell-quoting check. This only affects the string form of `CommandConfigOrString`; the structured `program`/`args` form is unaffected. Signed-off-by: Jean Mertz --- crates/jp_config/src/types/command.rs | 158 +++++++++++++++++++- crates/jp_config/src/types/command_tests.rs | 85 +++++++++++ 2 files changed, 241 insertions(+), 2 deletions(-) diff --git a/crates/jp_config/src/types/command.rs b/crates/jp_config/src/types/command.rs index e8ba761c..97cfb700 100644 --- a/crates/jp_config/src/types/command.rs +++ b/crates/jp_config/src/types/command.rs @@ -13,6 +13,14 @@ //! "echo 'hello world'" => ["echo", "hello world"] //! ``` //! +//! Minijinja template spans (`{{ … }}`, `{% … %}`, `{# … #}`) are treated as +//! atomic during the split, so an expression may contain spaces without being +//! torn across arguments: +//! +//! ```ignore +//! "just x {{ a | default('') }}" => ["just", "x", "{{ a | default('') }}"] +//! ``` +//! //! Malformed shell quoting (unbalanced quotes, dangling escapes) is rejected at //! config-parse time via [`PartialCommandConfigOrString::from_str`] rather than //! producing garbage at spawn time. @@ -95,7 +103,7 @@ impl FromStr for PartialCommandConfigOrString { // an empty `program`, which is the same behavior the old // `split_whitespace` parser had — execution-time error, not a // config-parse error). - if shlex::split(s).is_none() { + if split_command_words(s).is_none() { return Err(format!("invalid shell quoting in command string: {s:?}").into()); } @@ -112,6 +120,9 @@ impl CommandConfigOrString { /// [`CommandConfig::args`]. /// [`CommandConfig::shell`] is `false`. /// + /// Minijinja template spans (`{{ … }}`, `{% … %}`, `{# … #}`) stay in a + /// single argument even when they contain spaces. + /// /// Malformed input is normally rejected at config-parse time by /// [`PartialCommandConfigOrString::from_str`]. /// This method is defensive against direct construction in Rust code: on @@ -121,7 +132,7 @@ impl CommandConfigOrString { pub fn command(self) -> CommandConfig { match self { Self::String(v) => { - let mut iter = shlex::split(&v).unwrap_or_default().into_iter(); + let mut iter = split_command_words(&v).unwrap_or_default().into_iter(); CommandConfig { program: iter.next().unwrap_or_default(), @@ -247,6 +258,149 @@ impl ToPartial for CommandConfig { } } +/// Split a command string into shell words, treating minijinja template spans +/// as atomic. +/// +/// The shell split happens before template rendering, so a rendered value can +/// never inject extra arguments. +/// A template span may itself contain spaces (e.g. `{{ x | default('') }}`); +/// masking the spans before the split keeps each one inside a single argument. +/// Returns `None` on unbalanced shell quoting, mirroring [`shlex::split`]. +fn split_command_words(input: &str) -> Option> { + let (masked, spans) = mask_template_spans(input); + let words = shlex::split(&masked)?; + Some( + words + .into_iter() + .map(|word| restore_spans(&word, &spans)) + .collect(), + ) +} + +/// Replace every minijinja template span (`{{ … }}`, `{% … %}`, `{# … #}`) +/// with a NUL-delimited placeholder, returning the masked string and the +/// original span texts in order. +/// +/// A config value cannot carry a NUL byte, so the placeholder cannot collide +/// with real command text. +/// The placeholder holds no whitespace or quote characters, so `shlex` keeps it +/// inside a single word. +fn mask_template_spans(input: &str) -> (String, Vec) { + let bytes = input.as_bytes(); + let mut out = String::with_capacity(input.len()); + let mut spans: Vec = Vec::new(); + let mut i = 0; + + while i < bytes.len() { + let Some(open) = find_span_open(bytes, i) else { + out.push_str(&input[i..]); + break; + }; + + out.push_str(&input[i..open]); + let end = find_span_end(bytes, open); + out.push('\0'); + out.push_str(&spans.len().to_string()); + out.push('\0'); + spans.push(input[open..end].to_owned()); + i = end; + } + + (out, spans) +} + +/// Find the next `{{` / `{%` / `{#` opener at or after `from`. +fn find_span_open(bytes: &[u8], from: usize) -> Option { + (from..bytes.len().saturating_sub(1)) + .find(|&j| bytes[j] == b'{' && matches!(bytes[j + 1], b'{' | b'%' | b'#')) +} + +/// Find the byte index just past the closer of the span opened at `open`. +/// +/// Comment spans (`{# … #}`) are raw text. +/// Expression (`{{ … }}`) and statement (`{% … %}`) spans skip string +/// literals, so a closer inside a quoted string does not end the span early. +/// An unterminated span extends to end of input; the render step then reports +/// the real error. +fn find_span_end(bytes: &[u8], open: usize) -> usize { + match bytes[open + 1] { + b'#' => find_literal_close(bytes, open + 2, b'#'), + kind => { + let closer = if kind == b'{' { b'}' } else { b'%' }; + let mut j = open + 2; + let mut quote: Option = None; + + while j < bytes.len() { + let b = bytes[j]; + if let Some(q) = quote { + if b == b'\\' { + j += 2; + continue; + } + if b == q { + quote = None; + } + j += 1; + } else if b == b'\'' || b == b'"' { + quote = Some(b); + j += 1; + } else if b == closer && bytes.get(j + 1) == Some(&b'}') { + return j + 2; + } else { + j += 1; + } + } + + bytes.len() + } + } +} + +/// Find the byte index just past a literal `first`+`}` closer at or after `j`. +fn find_literal_close(bytes: &[u8], mut j: usize, first: u8) -> usize { + while j < bytes.len() { + if bytes[j] == first && bytes.get(j + 1) == Some(&b'}') { + return j + 2; + } + j += 1; + } + bytes.len() +} + +/// Substitute masked placeholders in `token` back to their original span text. +fn restore_spans(token: &str, spans: &[String]) -> String { + if !token.contains('\0') { + return token.to_owned(); + } + + let mut out = String::with_capacity(token.len()); + let mut rest = token; + + while let Some(start) = rest.find('\0') { + out.push_str(&rest[..start]); + let after = &rest[start + 1..]; + + let Some(end) = after.find('\0') else { + // Unbalanced marker (not one this masker produced): emit verbatim. + out.push('\0'); + rest = after; + continue; + }; + + match after[..end].parse::() { + Ok(idx) if idx < spans.len() => out.push_str(&spans[idx]), + _ => { + out.push('\0'); + out.push_str(&after[..=end]); + } + } + rest = &after[end + 1..]; + } + + out.push_str(rest); + out +} + #[cfg(test)] #[path = "command_tests.rs"] mod tests; diff --git a/crates/jp_config/src/types/command_tests.rs b/crates/jp_config/src/types/command_tests.rs index c38cc432..392fb5c2 100644 --- a/crates/jp_config/src/types/command_tests.rs +++ b/crates/jp_config/src/types/command_tests.rs @@ -113,3 +113,88 @@ fn test_command_config_structured_passthrough() { shell: true, }); } + +#[test] +fn template_span_with_spaces_stays_one_arg() { + // A template expression with interior spaces (and a quoted filter arg) must + // survive the shell split as a single argument. + let p = + PartialCommandConfigOrString::from_str("just rfd-renumber {{ a }} {{ b | default('') }}") + .unwrap(); + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + + assert_eq!(cfg.command(), CommandConfig { + program: "just".to_owned(), + args: vec![ + "rfd-renumber".to_owned(), + "{{ a }}".to_owned(), + "{{ b | default('') }}".to_owned(), + ], + shell: false, + }); +} + +#[test] +fn template_statement_and_comment_spans_are_atomic() { + let p = PartialCommandConfigOrString::from_str("run {% if x %} {# note #}").unwrap(); + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + + assert_eq!(cfg.command(), CommandConfig { + program: "run".to_owned(), + args: vec!["{% if x %}".to_owned(), "{# note #}".to_owned()], + shell: false, + }); +} + +#[test] +fn template_span_adjacent_to_literal_text() { + let p = PartialCommandConfigOrString::from_str("cmd pre{{ x }}post").unwrap(); + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + + assert_eq!(cfg.command(), CommandConfig { + program: "cmd".to_owned(), + args: vec!["pre{{ x }}post".to_owned()], + shell: false, + }); +} + +#[test] +fn template_span_closer_inside_string_literal() { + // The `}}` inside the quoted string must not end the span early. + let p = PartialCommandConfigOrString::from_str(r#"echo {{ "}}" }}"#).unwrap(); + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + + assert_eq!(cfg.command(), CommandConfig { + program: "echo".to_owned(), + args: vec![r#"{{ "}}" }}"#.to_owned()], + shell: false, + }); +} + +#[test] +fn unterminated_template_span_is_kept_whole() { + // An unterminated `{{` swallows the rest; minijinja reports the real error + // at render time rather than the splitter mangling it. + let p = PartialCommandConfigOrString::from_str("just x {{ a").unwrap(); + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + + assert_eq!(cfg.command(), CommandConfig { + program: "just".to_owned(), + args: vec!["x".to_owned(), "{{ a".to_owned()], + shell: false, + }); +} + +#[test] +fn from_str_ignores_quotes_inside_template_span() { + // The apostrophe inside the comment is minijinja text, not shell quoting; + // masking the span keeps the unbalanced-quote check from rejecting it. + let p = PartialCommandConfigOrString::from_str("echo {# don't split #}").unwrap(); + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + + assert_eq!(cfg.command(), CommandConfig { + program: "echo".to_owned(), + args: vec!["{# don't split #}".to_owned()], + shell: false, + }); +} From ead807c4670a91e6c3b5c2aa1f61f0eddc6f4bfe Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Thu, 30 Jul 2026 15:07:35 +0200 Subject: [PATCH 2/2] review feedback Signed-off-by: Jean Mertz --- crates/jp_config/src/editor_tests.rs | 25 ++++++++++ crates/jp_config/src/types/command.rs | 48 +++++++++++++++---- crates/jp_config/src/types/command_tests.rs | 53 +++++++++++++++++++++ 3 files changed, 116 insertions(+), 10 deletions(-) diff --git a/crates/jp_config/src/editor_tests.rs b/crates/jp_config/src/editor_tests.rs index 08325065..a5f397f4 100644 --- a/crates/jp_config/src/editor_tests.rs +++ b/crates/jp_config/src/editor_tests.rs @@ -152,6 +152,31 @@ fn command_cmd_table_forwards_appended_path() { assert_eq!(out, ""); } +/// `editor.cmd` is never rendered as a template, but the string form still +/// treats a minijinja span as atomic, so the span reaches the program as one +/// argument instead of being torn into shell words. +#[cfg(unix)] +#[test] +fn command_cmd_string_keeps_template_span_in_one_argument() { + let cfg = EditorConfig { + cmd: Some(string_cmd("printf '<%s>' {{ a b }}")), + envs: vec![], + inline: InlineEditorConfig::default(), + }; + + let out = cfg + .command() + .unwrap() + .before_spawn(|cmd| { + cmd.arg("FILE"); + Ok(()) + }) + .read() + .unwrap(); + + assert_eq!(out, "<{{ a b }}>"); +} + /// A missing program with `shell = false` is a spawn error, not a silent /// non-zero exit — this is the typo'd-`editor.cmd` case. #[cfg(unix)] diff --git a/crates/jp_config/src/types/command.rs b/crates/jp_config/src/types/command.rs index 97cfb700..2c671f8c 100644 --- a/crates/jp_config/src/types/command.rs +++ b/crates/jp_config/src/types/command.rs @@ -21,9 +21,16 @@ //! "just x {{ a | default('') }}" => ["just", "x", "{{ a | default('') }}"] //! ``` //! +//! Spans are atomic for every string-form command, not only for the ones that +//! are rendered: tool commands are rendered by +//! `jp_llm::tool::run_tool_command`, while `editor.cmd` runs verbatim and keeps +//! a span as a literal argument. +//! //! Malformed shell quoting (unbalanced quotes, dangling escapes) is rejected at //! config-parse time via [`PartialCommandConfigOrString::from_str`] rather than //! producing garbage at spawn time. +//! Quoting *inside* a span is minijinja text rather than shell syntax, so it is +//! not part of that check. use std::{fmt, str::FromStr}; @@ -97,6 +104,12 @@ impl FromStr for PartialCommandConfigOrString { type Err = BoxedError; fn from_str(s: &str) -> Result { + // NUL is reserved as the placeholder delimiter used while masking + // template spans, and no OS accepts it in an argument anyway. + if s.contains('\0') { + return Err(format!("command string contains a NUL byte: {s:?}").into()); + } + // Validate shell quoting at parse time so malformed input fails fast // rather than producing garbage at spawn time. Empty / whitespace-only // strings are accepted (they parse to an empty token list and produce @@ -125,9 +138,10 @@ impl CommandConfigOrString { /// /// Malformed input is normally rejected at config-parse time by /// [`PartialCommandConfigOrString::from_str`]. - /// This method is defensive against direct construction in Rust code: on - /// `shlex::split` failure it falls back to an empty token list, which - /// surfaces as a spawn-time error. + /// This method is defensive against input that skips that check (direct + /// construction in Rust code, or a config format that can encode a NUL + /// byte): a split failure falls back to an empty token list, which surfaces + /// as a spawn-time error. #[must_use] pub fn command(self) -> CommandConfig { match self { @@ -151,8 +165,7 @@ impl CommandConfigOrString { /// redirects). /// The `args` are shell-quoted with [`shlex::try_join`] so multi-word arguments /// keep their boundaries instead of being word-split by the shell (`try_join` -/// only fails on an interior NUL byte, which a config value can't carry; a raw -/// space-join is the fallback). +/// only fails on an interior NUL byte; a raw space-join is the fallback). /// /// The caller wraps the result in its own shell invocation (`sh -c `). #[must_use] @@ -265,8 +278,14 @@ impl ToPartial for CommandConfig { /// never inject extra arguments. /// A template span may itself contain spaces (e.g. `{{ x | default('') }}`); /// masking the spans before the split keeps each one inside a single argument. -/// Returns `None` on unbalanced shell quoting, mirroring [`shlex::split`]. +/// Returns `None` on unbalanced shell quoting (mirroring [`shlex::split`]) and +/// on an interior NUL byte, which the masking step reserves for its +/// placeholders. fn split_command_words(input: &str) -> Option> { + if input.contains('\0') { + return None; + } + let (masked, spans) = mask_template_spans(input); let words = shlex::split(&masked)?; Some( @@ -281,8 +300,8 @@ fn split_command_words(input: &str) -> Option> { /// with a NUL-delimited placeholder, returning the masked string and the /// original span texts in order. /// -/// A config value cannot carry a NUL byte, so the placeholder cannot collide -/// with real command text. +/// Callers reject input containing NUL (see [`split_command_words`]), so a +/// placeholder cannot collide with real command text. /// The placeholder holds no whitespace or quote characters, so `shlex` keeps it /// inside a single word. fn mask_template_spans(input: &str) -> (String, Vec) { @@ -319,7 +338,10 @@ fn find_span_open(bytes: &[u8], from: usize) -> Option { /// /// Comment spans (`{# … #}`) are raw text. /// Expression (`{{ … }}`) and statement (`{% … %}`) spans skip string -/// literals, so a closer inside a quoted string does not end the span early. +/// literals, so a closer inside a quoted string does not end the span early, +/// and track `()`, `[]` and `{}` nesting, mirroring minijinja, which only +/// accepts an end delimiter at nesting depth zero. +/// In `{{ {"a": 1} }}` the first `}` closes the map, not the span. /// An unterminated span extends to end of input; the render step then reports /// the real error. fn find_span_end(bytes: &[u8], open: usize) -> usize { @@ -329,6 +351,7 @@ fn find_span_end(bytes: &[u8], open: usize) -> usize { let closer = if kind == b'{' { b'}' } else { b'%' }; let mut j = open + 2; let mut quote: Option = None; + let mut depth: usize = 0; while j < bytes.len() { let b = bytes[j]; @@ -344,9 +367,14 @@ fn find_span_end(bytes: &[u8], open: usize) -> usize { } else if b == b'\'' || b == b'"' { quote = Some(b); j += 1; - } else if b == closer && bytes.get(j + 1) == Some(&b'}') { + } else if depth == 0 && b == closer && bytes.get(j + 1) == Some(&b'}') { return j + 2; } else { + match b { + b'(' | b'[' | b'{' => depth += 1, + b')' | b']' | b'}' => depth = depth.saturating_sub(1), + _ => {} + } j += 1; } } diff --git a/crates/jp_config/src/types/command_tests.rs b/crates/jp_config/src/types/command_tests.rs index 392fb5c2..cc7ba855 100644 --- a/crates/jp_config/src/types/command_tests.rs +++ b/crates/jp_config/src/types/command_tests.rs @@ -185,6 +185,59 @@ fn unterminated_template_span_is_kept_whole() { }); } +#[test] +fn template_span_with_nested_braces_stays_one_arg() { + // Minijinja only ends a variable block at nesting depth zero, so the `}}` + // that closes the two maps must not end the span. + let p = PartialCommandConfigOrString::from_str(r#"cmd {{ {"outer": {"inner": 1}} | tojson }}"#) + .unwrap(); + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + + assert_eq!(cfg.command(), CommandConfig { + program: "cmd".to_owned(), + args: vec![r#"{{ {"outer": {"inner": 1}} | tojson }}"#.to_owned()], + shell: false, + }); +} + +#[test] +fn template_span_ends_at_first_depth_zero_closer() { + // The span ends at the `}}` that follows the balanced map, and text after it + // is split as ordinary shell words. + let p = PartialCommandConfigOrString::from_str(r#"cmd {{ {"a": 1} }} tail"#).unwrap(); + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + + assert_eq!(cfg.command(), CommandConfig { + program: "cmd".to_owned(), + args: vec![r#"{{ {"a": 1} }}"#.to_owned(), "tail".to_owned()], + shell: false, + }); +} + +#[test] +fn from_str_rejects_nul_byte() { + let err = PartialCommandConfigOrString::from_str("echo \0").unwrap_err(); + assert!(err.to_string().contains("NUL byte"), "got: {err}"); +} + +#[test] +fn json_nul_bearing_command_is_not_rewritten_by_span_restoration() { + // JSON (and YAML, and JSON5) can encode an interior NUL, and that path + // deserializes the string variant directly without going through `from_str`. + // Text shaped like a span placeholder must not be substituted with real span + // text: the command stays unexecutable instead of turning into a different + // one. + let p: PartialCommandConfigOrString = + serde_json::from_str(r#""\u00000\u0000 {{ evil }}""#).unwrap(); + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + + assert_eq!(cfg.command(), CommandConfig { + program: String::new(), + args: vec![], + shell: false, + }); +} + #[test] fn from_str_ignores_quotes_inside_template_span() { // The apostrophe inside the comment is minijinja text, not shell quoting;