holm/nushell
1
1
Fork 0
forked from extern/nushell
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Trim quotes when shelling out to cmd.exe (#7740)

Browse Source 
Closes #6337 and #5366. Prior to this PR, when "shelling out" to cmd.exe
on Windows we were not trimming quotes correctly:

```bash
〉^echo "foo"
\"foo\"
```
After this change, we do:
```bash
〉^echo "foo"
foo
```

### Breaking Change

I ended up removing `dir` from the list of supported cmd.exe internal
commands as part of this PR.

For this PR, I extracted the argument-cleaning-and-expanding code from
`spawn_simple_command()` for reuse in `spawn_cmd_command()`. This means
that we now expand globs, which broke some tests for the `dir` cmd.exe
internal command.

I probably could have kept the tests working, but... tbh, I don't think
it's worth it. I don't want to make the `cmd.exe` functionality any more
complicated than it already is, and calling `dir` from Nu is always
going to be weird+hacky compared to `ls`.
This commit is contained in:
Reilly Wood 2023-01-13 11:00:30 -08:00 committed by GitHub
parent 3dd21c635a
commit 49ab559992
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 95 additions and 118 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

152
crates/nu-command/src/system/run_external.rs
View File

@ -202,9 +202,8 @@ impl ExternalCommand {
// This has the full list of cmd.exe "internal" commands: https://ss64.com/nt/syntax-internal.html
// I (Reilly) went through the full list and whittled it down to ones that are potentially useful:
const CMD_INTERNAL_COMMANDS: [&str; 10] = [
"ASSOC", "CLS", "DIR", "ECHO", "FTYPE", "MKLINK", "PAUSE", "START", "VER",
"VOL",
const CMD_INTERNAL_COMMANDS: [&str; 9] = [
"ASSOC", "CLS", "ECHO", "FTYPE", "MKLINK", "PAUSE", "START", "VER", "VOL",
];
let command_name_upper = self.name.item.to_uppercase();
let looks_like_cmd_internal = CMD_INTERNAL_COMMANDS
@ -527,7 +526,7 @@ impl ExternalCommand {
) -> Result<CommandSys, ShellError> {
let mut process = if let Some(d) = self.env_vars.get("PWD") {
let mut process = if use_cmd {
self.spawn_cmd_command()
self.spawn_cmd_command(d)
} else {
self.create_command(d)?
};
@ -578,7 +577,7 @@ impl ExternalCommand {
// We could give the option to call from powershell
// for minimal builds cwd is unused
if self.name.item.ends_with(".cmd") || self.name.item.ends_with(".bat") {
Ok(self.spawn_cmd_command())
Ok(self.spawn_cmd_command(cwd))
} else {
self.spawn_simple_command(cwd)
}
@ -599,76 +598,14 @@ impl ExternalCommand {
let mut process = std::process::Command::new(head);
for (arg, arg_keep_raw) in self.args.iter().zip(self.arg_keep_raw.iter()) {
// if arg is quoted, like "aa", 'aa', `aa`, or:
// if arg is a variable or String interpolation, like: $variable_name, $"($variable_name)"
// `as_a_whole` will be true, so nu won't remove the inner quotes.
let (trimmed_args, run_glob_expansion, mut keep_raw) = trim_enclosing_quotes(&arg.item);
if *arg_keep_raw {
keep_raw = true;
}
let mut arg = Spanned {
item: if keep_raw {
trimmed_args
} else {
remove_quotes(trimmed_args)
},
span: arg.span,
};
if !keep_raw {
arg.item = nu_path::expand_tilde(arg.item)
.to_string_lossy()
.to_string();
}
let cwd = PathBuf::from(cwd);
if arg.item.contains('*') && run_glob_expansion {
if let Ok((prefix, matches)) =
nu_engine::glob_from(&arg, &cwd, self.name.span, None)
{
let matches: Vec<_> = matches.collect();
// FIXME: do we want to special-case this further? We might accidentally expand when they don't
// intend to
if matches.is_empty() {
process.arg(&arg.item);
}
for m in matches {
if let Ok(arg) = m {
let arg = if let Some(prefix) = &prefix {
if let Ok(remainder) = arg.strip_prefix(prefix) {
let new_prefix = if let Some(pfx) = diff_paths(prefix, &cwd) {
pfx
} else {
prefix.to_path_buf()
};
new_prefix.join(remainder).to_string_lossy().to_string()
} else {
arg.to_string_lossy().to_string()
}
} else {
arg.to_string_lossy().to_string()
};
process.arg(&arg);
} else {
process.arg(&arg.item);
}
}
}
} else {
process.arg(&arg.item);
}
trim_expand_and_apply_arg(&mut process, arg, arg_keep_raw, cwd);
}
Ok(process)
}
/// Spawn a cmd command with `cmd /c args...`
pub fn spawn_cmd_command(&self) -> std::process::Command {
pub fn spawn_cmd_command(&self, cwd: &str) -> std::process::Command {
let mut process = std::process::Command::new("cmd");
// Disable AutoRun
@ -678,13 +615,17 @@ impl ExternalCommand {
process.arg("/c");
process.arg(&self.name.item);
for arg in &self.args {
// Clean the args before we use them:
for (arg, arg_keep_raw) in self.args.iter().zip(self.arg_keep_raw.iter()) {
// https://stackoverflow.com/questions/1200235/how-to-pass-a-quoted-pipe-character-to-cmd-exe
// cmd.exe needs to have a caret to escape a pipe
let arg = arg.item.replace('|', "^|");
process.arg(&arg);
let arg = Spanned {
item: arg.item.replace('|', "^|"),
span: arg.span,
};
trim_expand_and_apply_arg(&mut process, &arg, arg_keep_raw, cwd)
}
process
}
@ -702,6 +643,71 @@ impl ExternalCommand {
}
}
fn trim_expand_and_apply_arg(
process: &mut CommandSys,
arg: &Spanned<String>,
arg_keep_raw: &bool,
cwd: &str,
) {
// if arg is quoted, like "aa", 'aa', `aa`, or:
// if arg is a variable or String interpolation, like: $variable_name, $"($variable_name)"
// `as_a_whole` will be true, so nu won't remove the inner quotes.
let (trimmed_args, run_glob_expansion, mut keep_raw) = trim_enclosing_quotes(&arg.item);
if *arg_keep_raw {
keep_raw = true;
}
let mut arg = Spanned {
item: if keep_raw {
trimmed_args
} else {
remove_quotes(trimmed_args)
},
span: arg.span,
};
if !keep_raw {
arg.item = nu_path::expand_tilde(arg.item)
.to_string_lossy()
.to_string();
}
let cwd = PathBuf::from(cwd);
if arg.item.contains('*') && run_glob_expansion {
if let Ok((prefix, matches)) = nu_engine::glob_from(&arg, &cwd, arg.span, None) {
let matches: Vec<_> = matches.collect();
// FIXME: do we want to special-case this further? We might accidentally expand when they don't
// intend to
if matches.is_empty() {
process.arg(&arg.item);
}
for m in matches {
if let Ok(arg) = m {
let arg = if let Some(prefix) = &prefix {
if let Ok(remainder) = arg.strip_prefix(prefix) {
let new_prefix = if let Some(pfx) = diff_paths(prefix, &cwd) {
pfx
} else {
prefix.to_path_buf()
};
new_prefix.join(remainder).to_string_lossy().to_string()
} else {
arg.to_string_lossy().to_string()
}
} else {
arg.to_string_lossy().to_string()
};
process.arg(&arg);
} else {
process.arg(&arg.item);
}
}
}
} else {
process.arg(&arg.item);
}
}
/// Given an invalid command name, try to suggest an alternative
fn suggest_command(attempted_command: &str, engine_state: &EngineState) -> Option<String> {
let commands = engine_state.get_signatures(false);

4
crates/nu-command/tests/commands/redirection.rs
View File

@ -20,7 +20,7 @@ fn redirect_err() {
Playground::setup("redirect_err_test", |dirs, _sandbox| {
let output = nu!(
cwd: dirs.test(),
"dir missingapplication err> a; (open a | size).bytes >= 16"
"vol missingdrive err> a; (open a | size).bytes >= 16"
);
assert!(output.out.contains("true"));
@ -46,7 +46,7 @@ fn redirect_outerr() {
Playground::setup("redirect_outerr_test", |dirs, _sandbox| {
let output = nu!(
cwd: dirs.test(),
"dir missingapplication out+err> a; (open a | size).bytes >= 16"
"vol missingdrive out+err> a; (open a | size).bytes >= 16"
);
assert!(output.out.contains("true"));

57
crates/nu-command/tests/commands/run_external.rs
View File

@ -1,3 +1,4 @@
#[cfg(not(windows))]
use nu_test_support::fs::Stub::EmptyFile;
use nu_test_support::playground::Playground;
use nu_test_support::{nu, pipeline};
@ -212,49 +213,6 @@ fn external_command_not_expand_tilde_with_quotes() {
)
}
#[cfg(windows)]
#[test]
fn explicit_glob_windows() {
Playground::setup("external with explicit glob", |dirs, sandbox| {
sandbox.with_files(vec![
EmptyFile("D&D_volume_1.txt"),
EmptyFile("D&D_volume_2.txt"),
EmptyFile("foo.sh"),
]);
let actual = nu!(
cwd: dirs.test(), pipeline(
r#"
^dir | glob '*.txt' | length
"#
));
assert_eq!(actual.out, "2");
})
}
#[cfg(windows)]
#[test]
fn bare_word_expand_path_glob_windows() {
Playground::setup("bare word should do the expansion", |dirs, sandbox| {
sandbox.with_files(vec![
EmptyFile("D&D_volume_1.txt"),
EmptyFile("D&D_volume_2.txt"),
EmptyFile("foo.sh"),
]);
let actual = nu!(
cwd: dirs.test(), pipeline(
r#"
^dir *.txt
"#
));
assert!(actual.out.contains("D&D_volume_1.txt"));
assert!(actual.out.contains("D&D_volume_2.txt"));
})
}
#[cfg(windows)]
#[test]
fn failed_command_with_semicolon_will_not_execute_following_cmds_windows() {
@ -329,3 +287,16 @@ fn can_run_batch_files_without_bat_extension() {
},
);
}
#[cfg(windows)]
#[test]
fn quotes_trimmed_when_shelling_out() {
// regression test for a bug where we weren't trimming quotes around string args before shelling out to cmd.exe
let actual = nu!(cwd: ".", pipeline(
r#"
^echo "foo"
"#
));
assert_eq!(actual.out, "foo");
}