From c8f30fa3bfe6157808dde8cb7d4fda03cb254d61 Mon Sep 17 00:00:00 2001 From: Marika Chlebowska Date: Fri, 19 Jan 2024 17:20:14 +0100 Subject: [PATCH] Fix parsing of strings with special characters (#11030) # Description If there were brackets in a string argument of a script it was always interpreted as interpolation before the change. That lead to unexpected outputs of such scripts. After this change arguments which are not intended as interpolation (not starting with $) and containing brackets will have implicitly added backticks for correct interpretation in the scripts. This fixes #10908. To fix other issues mentioned in #11035 I changed the deparsing logic. Initially we added backticks for multi word variables and double quote if there was \ or " in the string. My change would add double quotes any time string starts with $ or contains any of character that might break parsing. The characters I identified are white space, (, ', `, ",and \. It's possible other characters should be added to this list. I tested this solution with few simple scripts using both stand alone arguments and flags and it seems to work but I would appreciate if someone with more experience checked it with some more unusual cases I missed. # User-Facing Changes Erroneous behaviour described in the issue will no longer happen. # Tests + Formatting Added tests for new formatting. # After Submitting --- crates/nu-parser/src/deparse.rs | 73 +++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 27 deletions(-) diff --git a/crates/nu-parser/src/deparse.rs b/crates/nu-parser/src/deparse.rs index e18da10ad8..6188fb3e66 100644 --- a/crates/nu-parser/src/deparse.rs +++ b/crates/nu-parser/src/deparse.rs @@ -1,3 +1,10 @@ +fn string_should_be_quoted(input: &str) -> bool { + input.starts_with('$') + || input + .chars() + .any(|c| c == ' ' || c == '(' || c == '\'' || c == '`' || c == '"' || c == '\\') +} + pub fn escape_quote_string(input: &str) -> String { let mut output = String::with_capacity(input.len() + 2); output.push('"'); @@ -14,37 +21,32 @@ pub fn escape_quote_string(input: &str) -> String { } // Escape rules: -// input argument contains ' '(like abc def), we will convert it to `abc def`. -// input argument contains --version='xx yy', we will convert it to --version=`'xx yy'` -// input argument contains " or \, we will try to escape input. +// input argument is not a flag, does not start with $ and doesn't contain special characters, it is passed as it is (foo -> foo) +// input argument is not a flag and either starts with $ or contains special characters, quotes are added, " and \ are escaped (two \words -> "two \\words") +// input argument is a flag without =, it's passed as it is (--foo -> --foo) +// input argument is a flag with =, the first two points apply to the value (--foo=bar -> --foo=bar; --foo=bar' -> --foo="bar'") +// +// special characters are white space, (, ', `, ",and \ pub fn escape_for_script_arg(input: &str) -> String { // handle for flag, maybe we need to escape the value. if input.starts_with("--") { if let Some((arg_name, arg_val)) = input.split_once('=') { // only want to escape arg_val. - let arg_val = if arg_val.contains(' ') { - format!("`{arg_val}`") - } else if arg_val.contains('"') || arg_val.contains('\\') { + let arg_val = if string_should_be_quoted(arg_val) { escape_quote_string(arg_val) - } else if arg_val.is_empty() { - // return an empty string - "''".to_string() } else { arg_val.into() }; + return format!("{arg_name}={arg_val}"); + } else { + return input.into(); } } - - if input.contains(' ') { - format!("`{input}`") - } else if input.contains('"') || input.contains('\\') { + if string_should_be_quoted(input) { escape_quote_string(input) - } else if input.is_empty() { - // return an empty string - "''".to_string() } else { - input.to_string() + input.into() } } @@ -55,15 +57,28 @@ mod test { #[test] fn test_not_extra_quote() { // check for input arg like this: - // nu b.nu 8 + // nu b.nu word 8 + assert_eq!(escape_for_script_arg("word"), "word".to_string()); assert_eq!(escape_for_script_arg("8"), "8".to_string()); } + #[test] + fn test_quote_special() { + // check for input arg like this: + // nu b.nu "two words" $nake "`123" + assert_eq!( + escape_for_script_arg("two words"), + r#""two words""#.to_string() + ); + assert_eq!(escape_for_script_arg("$nake"), r#""$nake""#.to_string()); + assert_eq!(escape_for_script_arg("`123"), r#""`123""#.to_string()); + } + #[test] fn test_arg_with_flag() { // check for input arg like this: - // nu b.nu linux --version=v5.2 - assert_eq!(escape_for_script_arg("linux"), "linux".to_string()); + // nu b.nu --linux --version=v5.2 + assert_eq!(escape_for_script_arg("--linux"), "--linux".to_string()); assert_eq!( escape_for_script_arg("--version=v5.2"), "--version=v5.2".to_string() @@ -76,23 +91,27 @@ mod test { } #[test] - fn test_flag_arg_with_values_contains_space() { + fn test_flag_arg_with_values_contains_special() { // check for input arg like this: - // nu b.nu test_ver --version='xx yy' --arch=ghi + // nu b.nu test_ver --version='xx yy' --separator="`" assert_eq!( escape_for_script_arg("--version='xx yy'"), - "--version=`'xx yy'`".to_string() + r#"--version="'xx yy'""#.to_string() ); assert_eq!( - escape_for_script_arg("--arch=ghi"), - "--arch=ghi".to_string() + escape_for_script_arg("--separator=`"), + r#"--separator="`""#.to_string() ); } #[test] fn test_escape() { // check for input arg like this: - // nu b.nu '"' - assert_eq!(escape_for_script_arg(r#"""#), r#""\"""#.to_string()); + // nu b.nu \ --arg='"' + assert_eq!(escape_for_script_arg(r#"\"#), r#""\\""#.to_string()); + assert_eq!( + escape_for_script_arg(r#"--arg=""#), + r#"--arg="\"""#.to_string() + ); } }