Fix quoting in wrapper script

[neiser]

The 'eval' introduced in #262 unfortunately breaks command invocations were a parameter contains spaces, such as

rg "my pattern with spaces"

as it would be called as rg my pattern with spaces. This can be fixed using the "${@@Q}" operator in bash, see https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

At least the above invocation then works correctly! This PR also fixes some other shell check warnings by using quoting of variables (just in case somebody has a space in $HOME)

[neiser]

@fsquillace I'm investigating the Travis CI build failure

/home/travis/.junest/usr/bin_wrappers/pacman: line 5: ${@@Q}: bad substitution

Locally it works for me though:

$ whereis pacman
pacman: /home/agr/.junest/usr/bin_wrappers/pacman
$ pacman --help
usage:  pacman <operation> [...]
operations:
    pacman {-h --help}
    pacman {-V --version}
    pacman {-D --database} <options> <package(s)>
    pacman {-F --files}    [options] [file(s)]
    pacman {-Q --query}    [options] [package(s)]
    pacman {-R --remove}   [options] <package(s)>
    pacman {-S --sync}     [options] [package(s)]
    pacman {-T --deptest}  [options] [package(s)]
    pacman {-U --upgrade}  [options] <file(s)>

use 'pacman {-h --help}' with an operation for available options

[fsquillace]

Thanks for looking into this. One thing is not clear, are you sure rg "my pattern with spaces" does not work for you? It does work me with the current code. Which shell are you using? Maybe there is some weird shell settings?

[neiser]

@fsquillace I'm using zsh 5.8 to execute rg "my pattern with spaces", the my Ubuntu 20.04 LTS has bash version version 5.0.17(1)-release and within junest the bash version is 5.1.16(1)-release.

I've also found why travis is not working: I think bash version 4.4 is required. I've pushed another commit to confirm this, unsure though if you're willing to move to this version.

Also, using rg "my pattern with spaces" to debug this is quite subtle, as it just prints that it can't find the paths pattern and with and spaces and still appears to search for the pattern my. That's why I havn't noticed the bug directly.

[neiser]

@fsquillace I found that zsh does have its own take on word splitting but I think that's not of importance here, as the magic happens within the wrapper script (which runs in bash). Would be good though to understand why it appears to work for you but not for me.

By the way, the pipeline with bash version 4.4 is now "successful", at least the tests didn't fail as far as I can tell. I think it still fails as it doesn't have secrets provided for docker login (because this is a pull request).

Let me know if you have an idea how to investigate this further. I think this patch is still required to get it to work on my machine.

[fsquillace]

Good, I still would like to know why it does not work for you first. Can you provide the actual error? What are the step to reproduce the bug? I would like to see if I can reproduce it too.

[neiser]

@fsquillace Sure, I was a bit terse in my description. So if I use a bin_wrapper script without the ${@@Q} patch, I observe that due the extra eval introduced in #262 the arguments are passed without proper quoting to the rg call.

For rg, as already discussed, it's a bit subtle to see an actual problem. So, I've written the following test case to illustrate the problem in isolation, consisting of two files, both running in bash (so I'm pretty sure it's not a zsh problem):

A test.sh script (plays the role of the bin wrapper script):

#!/usr/bin/env bash
echo "== Calling with quoting: "
eval ./cmd.sh "${@@Q}"
echo "== Calling without quoting: "
eval ./cmd.sh "$@"

A little "receiving command script" cmd.sh with the following content (plays the role of rg in our previous discussion). It just dumps the first two received parameters (if any):

#!/usr/bin/env bash
echo "Param1: $1"
echo "Param2: $2"

Now, to illustrate the bug, you can call ./test.sh 'some text' or ./test.sh "some text" and get the output:

$ ./test.sh "some text"    
== Calling with quoting: 
Param1: some text
Param2: 
== Calling without quoting: 
Param1: some
Param2: text

As one can see in the case without quoting, the one an only param 'some text' is incorrectly split up when passed down to ./cmd.sh. It's expected though for cmd.sh that the first param is the complete text and the second param is empty. So I'd say this "undesired parameter split up" is a bug, which is indeed fixed by ${@@Q}.

Also, it gets even worse with evil eval if you run it with nasty things like backticks (but within '...' to actually prevent execution):

$ ./test.sh '`some cmd`'
== Calling with quoting: 
Param1: `some cmd`
Param2: 
== Calling without quoting: 
./test.sh: line 5: some: command not found
Param1: 
Param2: 

Here, without quoting, the backticks are evaluated "too early" already in test.sh and this fails with ./test.sh: line 5: some: command not found. With quoting, the proper content of the first parameter is passed down to cmd.sh, as desired by the initial call. In contrast to the first illustration, it's really important here that some cmd is wrapped in '...' and not in "..." which would evaluate even earlier in the calling shell!

I hope that you can reproduce my findings and agree that for the wrapper script, some quoting is necessary to prevent eval doing evil things to the passed-down parameters. Also note that all scripts run in bash here, and I've even run the test case in bash itself (with the same result).

If you want to stay with bash 4.0, there's an alternative solution using printf

#!/usr/bin/env bash
echo "== Calling with quoting: "
eval ./cmd.sh "$(printf "%q" "$@")"
echo "== Calling without quoting: "
eval ./cmd.sh "$@"

See also https://stackoverflow.com/a/39463371

I've adapted the PR with a push force to use printf to keep bash 4.0 compatibility.

[fsquillace]

Thanks again for this change and for the fantastic explanation. Yes you are right, I can also reproduce the issue now. I put just two minor comments. I will merge to dev branch as soon as they are addressed.

In https://github.com/fsquillace/junest/issues/262 I used eval to overcome the problem of not preserving quotes in the JUNEST_ARGS variable, I am not sure if it is possible to solve it in the same way, otherwise is fine.

I really appreciate your contribution!

[fsquillace]

Merged! Maybe at some point (when I have time!) I will add shellcheck. I think it will make codebase easier to maintain: https://github.com/fsquillace/junest/issues/288

Thanks again @neiser !

[neiser]

@fsquillace I've just pushed another proposal where I wanted instead of quoting limit the "blast radius" of eval introduced in #262. I'll open another PR for that. I didn't expect you'll merge this so quickly :)

[fsquillace]

Apparently this merge does not work :/ https://github.com/fsquillace/junest/issues/292

[neiser]

@fsquillace I think it's really time for some thorough unit tests. Right now I don't have much spare time, but with a little luck I could investigate upcoming week.

[fsquillace]

Your opened PR (without printf) seems to solve this problem instead if we compare with the previous commits:

rm ~/.junest/usr/bin_wrappers/pacman
junest -- exit
bash -x ~/.junest/usr/bin_wrappers/pacman -Rsn neovim
+ eval 'junest_args_array=(ns)'
++ junest_args_array=(ns)
+ junest ns -- pacman -Rsn neovim

[fsquillace]

This change fix the problem: https://github.com/fsquillace/junest/commit/73b8bec8db34d1cfb51bf97934ac31a3909f0eaf

[neiser]

@fsquillace Ah I see. I decided to use printf to avoid bumping the minimum required bash version AFAICR. Also doesn't this change re-introduce the bug I originally wanted to fix with quoting? The test doesn't seem to cover arguments containing spaces, doesn't it?

[fsquillace]

Can you double check those cases too? I think the test cover them. The test case sets the JUNEST_ARGS containing multiple arguments including the --bind option. Also, for the test, the command wrapper arguments contain an argument with spaces too. I did some direct manual tests too and it works. I am pretty sure there will be other use cases not covered given that dealing with arguments in bash is a complete nightmare.