4845: Can’t pass arguments to rbt alias in a safe manner

amiryal

What version are you running?

RBTools 1.0.2

What's the URL of the page containing the problem?

https://github.com/reviewboard/rbtools/commit/fb7e8d33182797bc681b17040d36cdc9a202c504

What steps will reproduce the problem?

  1. Define these aliases in .reviewboardrc:

ALIASES = {
    'shell': "!rbt test '--summary=Hello World'",
    'no-shell': "test '--summary=Hello World'",
}

Note: '--summary=Hello World' is a surrogate for shlex.quote("--summary={}".format(user_controlled_data)).

  1. Dry-run the aliases:

$ rbt alias --dry-run shell | sed -e 's/ //g' -e 's/""/ /g'
rbt test --summary=Hello World
$ rbt alias --dry-run no-shell                                                                      
rbt test "'--summary=Hello World'"

What is the expected output? What do you see instead?

The expected behaviour is to pass the summary as exactly one argument to the spawned process, and that the spawned process will not see the quotation marks surrounding the argument.

This is how the expected behaviour shold look like:

$ rbt alias --dry-run shell | sed -e 's/ //g' -e 's/""/ /g'
rbt test '--summary=Hello World'
$ rbt alias --dry-run no-shell                                                                      
rbt test '--summary=Hello World'

What operating system are you using?

Linux, Python 3.7

Please provide any additional information below.

In the first example (rbt shell), the arguments effectively go through:

' '.join(shlex.split(alias, posix=True)

This is incorrect because the inverse of shlex.split is ' '.join(map(shlex.quote)):

' '.join(map(shlex.quote, shlex.split(alias, posix=True)))

In the second example (rbt no-shell), the arguments effectively go through:

[RB_MAIN] + shlex.split(alias, posix=False)

The problem here is that shlex.split(posix=False) treats quotation marks in a counterproductive way. I honestly don’t understand what the use case for this mode is, and its documentation also fails to explain it. Maybe someone can dig it up from a PEP, or something.

The default $EFS in the shells that I tested with is " \t\n", so I can probably use the no-shell variant with something like:

def shell_quote(s):
    return (s
            .replace(' ', r'\ ')
            .replace('\t', r'\t')
            .replace('\n', r'\n'))

However, I would really like to not reinvent anything and just use shlex.quote on my user-controlled data.

#1 amiryal

Actually, my shell_quote implementation can’t work, even if I add handling of quotation characters and backslashes, because I could not find a way to escape newline characters.