shell_quote("'ab?'") should return '"\'ab?\'"', not "'ab?'"

[David Lowe]

Current behaviour:

>>> shell_quote("'ab?'")
 "'ab?'"
>>> print(shell_quote("'ab?'"))
'ab?'

Expected behaviour:

 >>> shell_quote("'ab?'")
 '"\'ab?\'"'
 >>> print(shell_quote("'ab?'"))
 "'ab?'"

For example, to create a file named 'ab?', that is, a filename that consists of a single quote character, the a character, the b character, the ? character and the single quote character, in bash you would type the following:

 $ touch "'ab?'"

And not the following:

 $ touch 'ab?'

[Vinay Sajip]

The point of shell_quote is to quote a string to be safe for the shell, not to return the value that you would type into bash. If the passed-in value is delimited by ' and ', it is returned as is, because it is considered safe. Can you give more info about what you are trying to achieve at a higher level than just the quoting behaviour?

[David Lowe]

Thanks for your quick response.

I don't understand what you mean by safe for the shell.

I want to be able to run the following command safely:

filename = "'ab?'"
subprocess.call("touch -- %s" % shell_quote(filename), shell=True)

Right now, this code does not do as expected. It touches the file ab?, instead of the file 'ab?'. I would want shell_quote to protect me from any type of weird filenames, including filenames that have quotes in them. The only exception to this is filenames that begin with a dash -, which can't be made safe using escaping.

[Vinay Sajip]

I'm very sorry for taking so long to get back to this - it slipped under my radar.

By "safe for the shell", I mean against shell injection attacks.

You can achieve the effect you desire by e.g.

sarge.run('touch "\'ab?\'"')

which will indeed touch the file 'ab?' and not the file ab?.

[David Lowe]

So if I want my code to be safe for the shell, I should not use shell_quote? What's the point of shell_quote, then?

Again, take this code:

filename = "'ab?'"
subprocess.call("touch -- %s" % shell_quote(filename), shell=True)

The expected behaviour is that a file named 'ab?' is created. What the code actually does is create a file named ab?. This is a bug. shell_quote was meant to protect me from this behaviour and it did not.

Telling me to work around the bug by not using the shell_quote function does not resolve the bug.

[David Lowe]

To be even more clear, here's an example of a shell injection attack:

filename = "'cute' ; rm -f attacked ; true 'kitten'"
subprocess.call("touch -- %s" % shell_quote(filename), shell=True)

The expected behaviour was that a file named 'cute' ; rm -f attacked ; true 'kitten' would be created in the current directory (that is a valid filename on Linux at least).

Instead, a file named cute was created, and a file named attacked was deleted.

[Vinay Sajip]

I didn't say anything about whether you should or shouldn't use shell_quote. You have indicated an unsafe string which shell_quote passes, which I agree is a bug; I was answering your original point by saying that you didn't specifically need to use shell_quote for the use case you mentioned.

[Vinay Sajip]

Resolves #2: added test for shell_quote change.

→ <<cset 60ce592cda1c>>

[David Lowe]

Thanks for the fix!