Use shlex.quote for quoting shell arguments

[jwodder]

There are several points in the code in which filenames used as arguments in shell commands (executed with either os.system() or subprocess.Popen(shell=True)) are quoted by just enclosing them in double quotes. This will fail whenever the filenames happen to already contain double quotes. This patch fixes this behavior by quoting the filenames with shlex.quote() instead (pipes.quote() in Python 2).

fixes #1026

[davidism]

Thanks for starting this!

This is sort of papering over a bigger design issue, which is the use of os.system. We should be using subprocess.call or subprocess.Popen and passing a list of args, which doesn't need special handling for quoting. I'm not even sure if shlex.quote is valid on Windows. This would require significantly more refactor though. Is that something you're willing to work on?

[jwodder]

@davidism Yes, I can do that. I just have a couple questions about the intentions of some os.system calls:

• os.system('(less) 2>/dev/null') == 0 in click._termui_impl.pager(): I know that the shell syntax means "Run less in a subshell and discard stderr," but I'm not clear why less is being run in a subshell in the first place. I could emulate this with subprocess.call(['sh', '-c', 'less'], stderr=subprocess.DEVNULL), but if (as I suspect) this whole thing is supposed to do nothing more than test whether the less command is available, the system/subprocess call can be scrapped entirely and replaced with shutil.which() (or a recreation thereof for Python 2).

• os.system('more "%s"' % filename) == 0 in click._termui_impl.pager(): Is this expression, like the above, only meant to check whether the more command is available? Then we can replace it with shutil.which().

• The os.system() calls in click._termui_impl.open_url(): The code for constructing the argument string deletes any double-quotes from the URL. Is this just solely a crude way of avoiding quoting errors, or is it desirable for some other reason as well? The way the code is now, it looks like it will fail if you ask it to open a URL that legimately contains a double-quote. (Also, I'm not 100% sure what the correct way to split apart the Windows argument string into a list is.)

[davidism]

the system/subprocess call can be scrapped entirely and replaced with shutil.which() (or a recreation thereof for Python 2).

Yes, I agree it seems overly complex to just check if it exists. Since there's no comments about why that was used, and simpler commands seem to accomplish the same thing, I don't mind replacing it. I don't want to copy the which implementation into Click though, maybe just fake it on Python 2 with checking the result of subprocess.call.

open_url ... The way the code is now, it looks like it will fail if you ask it to open a URL that legimately contains a double-quote.

The unquote function refers to URL quoting, which is something different, but the replace after that does look like it can be removed.

---

I opened #1476 for replacing os.system with subprocess, can you open another PR against that?

[davidism]

I'm going to merge this in to 7.1, but it should eventually be replaced with #1477. I added shlex_quote to the other places I could see that might get a value with spaces. I also went ahead and removed the replace('"', "") calls, since shlex_quote handles escaping internal quotes.

[sphuber]

We are experiencing the same regression as @dufferzafar although in our case it is maybe a bit because we were abusing the liberty of the interface. For unit testing of interactive components of our CLI that require the user to make changes in a visual editor, we would actually set the os.environ['EDITOR'] to something like 'sleep 1 ; vim -c "g!/^#=/s/$/Test" -cwq', such that the editing was made non-interactive. However, as noted above, the wrapping of editor in shlex_quote now makes the arguments part of the command as interpreted by bash and it fails to find the command.

I realize that our approach is a bit hacky: do you have an alternate idea on how to unit test interactive commands that go through an editor?

[davidism]

Click 7.1.2 is available on PyPI.