ZSH completion fails when descriptions contain quotes or <'s

[ephur]

We have an internal tool I'm trying to generate completions for, and some of the option strings include things like:

--opt "device where name=<customerid>-foo" 

In this case, ZSH is unable to load the generated completions due to the < being unescaped. Outputting to a file and giving a quick replace of all instances of < with \< allowed this to work.

The second problem case is where options contain single quotes in the usage description for example, an option:

--opt "a "string" to use" 

causes the following ZSH error:
#> tool param:name:delete --
_tool.phar:181: bad set of key/value pairs for associative array
_tool.phar:181: bad set of key/value pairs for associative array
_tool.phar:181: bad set of key/value pairs for associative array

I believe the "'s also need escaped when the completions are rendered.

[ktomk]

Thanks for the report, I wonder this hasn't been reported earlier.

Can you provide a description file that generates these error cases with this report? Otherwise its hard to properly take a look. @ephur

[ephur]

First, shows how the error and how I worked around with the <'s, the "'s aren't so easy. Below the images is snippets of the output as well as what that looks like in the codebase for this project:

One of the generated snippets which includes both unescaped < and quotes in the usage that cause the opts string to get that bad associated array error

    dbpool:create)
    opts+=("--ldap-username:Explicitly define the LDAP username which you are trying to use. \(overrides the environment variable if provided\)\ If none is provided, the application will attempt to retrieve REDACTED" "--database:The database type\(s\) which you are trying to interact with. \(e.g. --database="testDB"\)" "--prefix:Prefix for all dbpool names created in this command. \(i.e. "<prefix\>-\{REDACTED,REDACTED\}"\)" "--force:Create DBPool regardless of if it already exists or not.")
    ;; 

The code snippet where this is defined is here:

[ephur]

I'm personally still pretty new to the Symfony framework, I was able to solve the <'s issue by using strip_tags, but when I tried to also replace " with \" it was not suitable and replaced to many quotes. This project is pinned to Symfony 3.4 (which I understand is the same as 4.0 without deprecations removed)

[ktomk]

Is the phar available for testing? A stripped down version that autocomplete could read the data from would be fine.

My knowledge about Symfony is more from the side of what not works with specific versions as I use it with tools that use it, but not directly.

So with a phar at hand it would be possible to verify the ZSH console code is generated correctly to prevent errors in the shell. From first glance this looks like an injection issue to me and I would like to verify that.

[ephur]

It's not available, unfortunately it's an internal tool and has a lot of business info within the source.

[ktomk]

Assumed so, which my suggestion was to strip it. I need at least something to reproduce, otherwise this is too much guesswork.

/E: A minimal example would be best.

[ephur]

I think I can reproduce it with a stubbed out application that can be shared, I'll work on that but probably be a couple of days. I appreciate how responsive you have been and your willingness to work with a vague bug report!

[rquadling]

Hi. I'm not a zsh user so not set all of that up.

I think though, the following patch should work ...

diff --git a/src/DumpCommand.php b/src/DumpCommand.php
index d15ca46..9ef33e1 100644
--- a/src/DumpCommand.php
+++ b/src/DumpCommand.php
@@ -153,10 +153,17 @@ class DumpCommand extends Command
         $helpers = array(
             'zsh_describe' => function($value, $description = null) {
                 $value = '"'.str_replace(':', '\\:', $value);
+
+                // Temporarily replace " with something replaceable after escapeshellcmd
+                $description = str_replace('"', '__DOUBLE_QUOTE__', $description);
+
                 if (!empty($description)) {
                     $value .= ':'.escapeshellcmd($description);
                 }
 
+                // Restore and escape "
+                $value = str_replace('__DOUBLE_QUOTE__', '\"', $value);
+
                 return $value.'"';
             }
         );

When I do now attempt to run . against the generated output when running within zsh, I get ...

wu_a:710: command not found: compdef

(wu_a is the output of bin/symfony-autocomplete ~/dt/app3/wu --shell zsh > wu_a) ...

Before I was getting errors relating to the quote errors. Now it's the lack of the compdef command which is probably correct for me as I've not get completion enabled for zsh.

In the wu_a file, I see:

or one of the environments present in the configuration file referenced by <info\>--phinx-configuration</info\>" "--phinx-configuration:The Phinx configuration file to be loaded" "--clone:Clone the client\'s data" "--database-name:Override the name of the database \(only applicable when cloning multiple companies in a single command\)" "--dry-run:If enabled, when used with <info\>--clone</info\> this command will display the queries and relevant commands, but will not execute them.\
NOTE: This only applies to the queries and commands that will make changes in some way.\
<prefix\>-\"pingpong\"")

Note the escaped \"!

If you can manually test this, then I can get a PR made and released.

[rquadling]

I've created #68

Hopefully someone with good zsh knowledge can evaluate this.

If anyone can improve tests on this as I don't quite get how the BATS system works.

[ktomk]

Thanks for the push into this, much appreciated @rquadling.

On first glance from my point of view, the proposed changes confirm what I suspected that the encoding is currently insufficient for zsh, and you identified double-quotes in the description being the cause. Correct me if I'm wrong, but I'd say fair point

Unfortunately the original report is (still) lacking a reproducible way to trigger the issue so it is not possible to test it.

So if I may ask directly, were you able to reproduce the issue and if so, how?

---

If anyone can improve tests on this as I don't quite get how the BATS system works.

This is certainly possible. BATS is bash based. So apart from clarifying for your own "how" understanding, my first question would be if your environment is the bash shell and if you're able to execute the BATS suite at all.

All PR checks have passed (28) right away, so what would you consider an improvement to the tests in regard to your changes? I can imagine we perhaps should run them (for zsh) in a dedicated zsh environment, but then again I'm (personally) missing simple reproducibility from the (original) report to do so.

---

Alternatively as (surprisingly) no-one else has not complained earlier on, we could just merge and go on and see if anyone does complain then. This would be a way at least to progress further on with this issue.

Please let me know what you prefer.

[rquadling]

I was able to partially reproduce the issue locally by amending one of my Symfony console apps and adding <prefix\>-"pingpong" to the description of an option and then running the amended code through zsh (it was just some junk really ... I initially thought < and/or > may be the issue, but it wasn't, so then I added the "pingpong" as that's nice and simple to search for in the output from symfony-console-autocomplete as I have several hundred commands in my application).

But getting autocomplete to then use that ... that's the bit I don't want to start playing with.

I did add a comment above on how I managed to at least get the output from symfony-console-autocomplete to be "sourced" ... but I don't know if that's enough.

[ktomk]

@rquadling actually that sounds really promising. Similarly I started to create a stub symfony console app for testing. It is pretty explorative right now, in the end with your description this should come close to reproduce and also test for this issue.

I can already provoke some errors (and also worked around some to get the test keep going), I see some room for error conditions that need to be trapped and also on how to improve the tests.

foo-cli.php foo:bar --......    
_foo-cli.php:16: bad set of key/value pairs for associative array
_foo-cli.php:16: bad set of key/value pairs for associative array

(this is interesting as it is from a stock FooCommand that ships with (perhaps dev) symfony/console.)

Right now it's all in the shell, when I find out more, I might be able to push a branch or already integrate for the tests with master. I'm also chewing on how to make/test it without interaction. Potentially there are command injections possible, but I'm only at the beginning.

[rquadling]

In looking at the output that this tool generates, it really does just look like an unescaped double quote issue. Now... if the autocompletion tool for zsh doesn't understand escaped quotes (i.e. the fix I've put up) then that's a separate issue and another sort of substitution is required.

[ktomk]

Yes, this is also what I think with my first looks. Basically "encoding issues", here currently debugging the output which is ZSH shell code and then reading manuals for syntax and intended functionality.

if the autocompletion tool for zsh doesn't understand escaped quotes

For string outputs (within those scripts generated), my current suggestion would be to encode neither as double nor single quoted strings but as what a ZSH user guide calls POSIX quotes (5.1.3: POSIX quotes), this is not an official name, it is available in Bash as well (3.1.2.4 ANSI-C Quoting) and this is likely a better name.

This way of quoting is available both in Bash and ZSH (plus a couple of other shells, but AFAIK not Fish (/E: checked, no $'...' in Fish, but unquoted support for ANSI-C quoting: Quoting - Fish for bash users - fish-shell 3.3.1 documentation [I'd suggest to leave Fish out for the moment, as well as Bash])).

A reference to POSIX is 0000249: Add standard support for $'...' in shell, IIRC it's not yet in the standard (/E: checked, not in 2. Shell Command Language; The Open Group Base Specifications Issue 7, 2018 edition which is latest at time of writing). Nevertheless, the benefit of ANSI-C Quoting is that single-, double-quotes and control characters can be encoded as one string which I think is of benefit for the auto-completion generation in tackling with the issues uncovered.

PHPs' addcslashes() should do most of the legwork to encode with ANSI-C Quoting, only wrapping in $'...' should be left.

If you can read something out of my quickly written sentences above, what do you think about giving this shell string quoting/encoding a try for the open PR? This would prevent double encoding only to decode once back (as I read the PR) and if it works out well for ZSH autocompletion it is likely also a fix for the Bash one - given it's an issue there (but the longer I look into this issue with you, the more I'm sure it's small a bug-cluster). @rquadling