Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

External command parameters get mangled #6399

Closed
panicbit opened this issue Aug 24, 2022 · 12 comments · Fixed by #6420
Closed

External command parameters get mangled #6399

panicbit opened this issue Aug 24, 2022 · 12 comments · Fixed by #6420
Labels
external-commands Issues related to external commands quoting/expansion Issues related to string quoting and expansion of variable or glob patterns
Milestone

Comments

@panicbit
Copy link
Contributor

panicbit commented Aug 24, 2022

Describe the bug

External command paramaters get stripped of their outmost single quotes when their prefix contains an =.

How to reproduce

  • ^echo "= 'hi'"
  • ^echo "foo=bar 'hi'"

image

Expected behavior

I expect the parameters do behave the same as with non-external commands:

  • echo "= 'hi'"
  • echo "foo=bar 'hi'"

image

Configuration

key value
version 0.67.1
branch
commit_hash 0afe1e4
build_os linux-x86_64
build_target x86_64-unknown-linux-gnu
rust_version rustc 1.63.0 (4b91a6ea7 2022-08-08)
rust_channel stable-x86_64-unknown-linux-gnu
cargo_version cargo 1.63.0 (fd9c4297c 2022-07-01)
pkg_version 0.67.1
build_time 2022-08-24 11:35:06 +02:00
build_rust_channel release
features database, dataframe, default, trash, which, zip
installed_plugins
@fdncred
Copy link
Collaborator

fdncred commented Aug 24, 2022

I thought there was a PR a few weeks ago that stripped quotes like this on purpose?
I think this is the one I'm thinking about #5846

@panicbit
Copy link
Contributor Author

I think the idea was to only strip it for flags, which I can understand somewhat, but I don't think stripping all quotes for any arg was the intention.

@panicbit
Copy link
Contributor Author

Maybe it's important to note that this issue affects a real life use case (in fact, one of my scripts at work broke due to the change):

let dump_command = $"PGPASSWORD='($db_secret)' pg_dump -Fc -h '($db.host)' -p '($db.port)' -U postgres -d '($db.name)' > '/tmp/($dump_name)'"

^kubectl exec -n $bastion.namespace $bastion_pod -- sh -c $dump_command

@fdncred
Copy link
Collaborator

fdncred commented Aug 24, 2022

I think the idea was to only strip it for flags, which I can understand somewhat, but I don't think stripping all quotes for any arg was the intention.

Ya, you're probably right. Your issue just made me think of where we remove quotes and that was the only one I remember.

@fdncred
Copy link
Collaborator

fdncred commented Aug 24, 2022

I'm not sure what changed to break your script. Feel free to look at PRs. Maybe it was a mistake or a bug.

I've started using parameters like this because it seems to work better.

^external_command [$arg1 $arg2 $arg3]

@panicbit
Copy link
Contributor Author

panicbit commented Aug 24, 2022

In what ways does that work better? For the issue at hand at least it does not seem to matter:

image

@fdncred
Copy link
Collaborator

fdncred commented Aug 24, 2022

i guess i don't have anything that i call with parameters like this

@sholderbach sholderbach added external-commands Issues related to external commands quoting/expansion Issues related to string quoting and expansion of variable or glob patterns labels Aug 25, 2022
@hustcer hustcer added this to the v0.68.0 milestone Aug 26, 2022
@WindSoilder
Copy link
Collaborator

WindSoilder commented Aug 26, 2022

@panicbit Would you please verify if the latest main works for you with the following example?

let dump_command = $"PGPASSWORD='($db_secret)' pg_dump -Fc -h '($db.host)' -p '($db.port)' -U postgres -d '($db.name)' > '/tmp/($dump_name)'"

^kubectl exec -n $bastion.namespace $bastion_pod -- sh -c $""$dump_command""

@panicbit
Copy link
Contributor Author

panicbit commented Aug 26, 2022

It does work (although the correct incantation is $""($dump_command)""), but I'm not gonna lie, I hope this is not the final solution for this problem, because this feels pretty insane.
Theoretically I would have to do $""arg"" with every parameter, just to make sure it doesn't get mangled.
I will probably never complain about having to put quotes around variable uses in other shells anymore…

@WindSoilder
Copy link
Collaborator

WindSoilder commented Aug 26, 2022

Yeah I agreed.

Sorry for the current situation. I think it's better to use just $dump_command

@WindSoilder
Copy link
Collaborator

@panicbit Hi, sorry for disturb, would you please check the latest main again?

I think $dump_command can be used directly for now

let dump_command = $"PGPASSWORD='($db_secret)' pg_dump -Fc -h '($db.host)' -p '($db.port)' -U postgres -d '($db.name)' > '/tmp/($dump_name)'"

^kubectl exec -n $bastion.namespace $bastion_pod -- sh -c $dump_command

@panicbit
Copy link
Contributor Author

It works 🥳. Thanks for caring about this and putting in all the effort! 😃

IanManske pushed a commit that referenced this issue May 23, 2024
This PR is a complete rewrite of `run_external.rs`. The main goal of the
rewrite is improving readability, but it also fixes some bugs related to
argument handling and the PATH variable (fixes
#6011).

I'll discuss some technical details to make reviewing easier.

## Argument handling

Quoting arguments for external commands is hard. Like, *really* hard.
We've had more than a dozen issues and PRs dedicated to quoting
arguments (see Appendix) but the current implementation is still buggy.

Here's a demonstration of the buggy behavior:

```nu
let foo = "'bar'"
^touch $foo            # This creates a file named `bar`, but it should be `'bar'`
^touch ...[ "'bar'" ]  # Same
```

I'll describe how this PR deals with argument handling.

First, we'll introduce the concept of **bare strings**. Bare strings are
**string literals** that are either **unquoted** or **quoted by
backticks** [^1]. Strings within a list literal are NOT considered bare
strings, even if they are unquoted or quoted by backticks.

When a bare string is used as an argument to external process, we need
to perform tilde-expansion, glob-expansion, and inner-quotes-removal, in
that order. "Inner-quotes-removal" means transforming from
`--option="value"` into `--option=value`.

## `.bat` files and CMD built-ins

On Windows, `.bat` files and `.cmd` files are considered executable, but
they need `CMD.exe` as the interpreter. The Rust standard library
supports running `.bat` files directly and will spawn `CMD.exe` under
the hood (see
[documentation](https://doc.rust-lang.org/std/process/index.html#windows-argument-splitting)).
However, other extensions are not supported [^2].

Nushell also supports a selected number of CMD built-ins. The problem
with CMD is that it uses a different set of quoting rules. Correctly
quoting for CMD requires using
[Command::raw_arg()](https://doc.rust-lang.org/std/os/windows/process/trait.CommandExt.html#tymethod.raw_arg)
and manually quoting CMD special characters, on top of quoting from the
Nushell side. ~~I decided that this is too complex and chose to reject
special characters in CMD built-ins instead [^3]. Hopefully this will
not affact real-world use cases.~~ I've implemented escaping that works
reasonably well.

## `which-support` feature

The `which` crate is now a hard dependency of `nu-command`, making the
`which-support` feature essentially useless. The `which` crate is
already a hard dependency of `nu-cli`, and we should consider removing
the `which-support` feature entirely.

## Appendix

Here's a list of quoting-related issues and PRs in rough chronological
order.

* #4609
* #4631
* #4601
  * #5846
* #5978
  * #6014
* #6154
  * #6161
* #6399
  * #6420
  * #6426
* #6465
* #6559
  * #6560

[^1]: The idea that backtick-quoted strings act like bare strings was
introduced by Kubouch and briefly mentioned in [the language
reference](https://www.nushell.sh/lang-guide/chapters/strings_and_text.html#backtick-quotes).

[^2]: The documentation also said "running .bat scripts in this way may
be removed in the future and so should not be relied upon", which is
another reason to move away from this. But again, quoting for CMD is
hard.

[^3]: If anyone wants to try, the best resource I found on the topic is
[this](https://daviddeley.com/autohotkey/parameters/parameters.htm).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-commands Issues related to external commands quoting/expansion Issues related to string quoting and expansion of variable or glob patterns
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants