bad character range a-b at position X in _expand_args when brackets included

[interifter]

After upgrade from click-7.x to click-8.x, we are experiencing a failure due to glob blindly parsing arguments that include regex patterns.

Example Input: (see minimum code example, below)

# Click 7.x
> python click_basic.py demo --values "x=[f-1,2]"

# Works just fine, passing the full value 'x=[f-1,2]' to the click command

# Click 8.x
> python click_basic.py demo --values "x=[f-1,2]"
>   File "C:\Code\tmp\click_basic.py", line 18, in <module>
    test()
  File "C:\Code\tmp\venv\lib\site-packages\click\core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "C:\Code\tmp\venv\lib\site-packages\click\core.py", line 1040, in main
    args = _expand_args(args)
  File "C:\Code\tmp\venv\lib\site-packages\click\utils.py", line 572, in _expand_args
# Skipping several lines
...
# Continuing to last line
re.error: bad character range f-1 at position 7

This was rootcaused to a new line inside the new helper click/utils.py::_expand_args:

    from glob import glob, escape

    out = []

    for arg in args:
        
        if user:
            arg = os.path.expanduser(arg)

        if env:
            arg = os.path.expandvars(arg)
        # This causes the issue
        matches = glob(arg, recursive=glob_recursive)


        if not matches:
            out.append(arg)
        else:
            out.extend(matches)

    return out

Minimum Working Example

import click


@click.group()
def test():
    return 0

@test.command("demo")
@click.option(
    "--values",
    multiple=True,
    help="List of valid values for a particular property",
)
def demo(values):
    print(values)

if __name__ == "__main__":
    test()
else:
    test()

> python .\click_basic.py demo --values "x=[f-1,2]"

A couple things, here:

1. This function is blindly passing in strings without sanitizing - which is a potential security vulnerability
2. If we do want to support regex, it should be at the CLI level, not buried deep inside the click parsing logic

Proposal
Sanitize the call to glob:

# instead of this:
matches = glob(arg, recursive=glob_recursive)

# Do this, add escape
matches = glob(escape(arg), recursive=glob_recursive)

This removes the security vulnerability and doesn't do some hidden regex underneath the hood, before data makes it to the user.

It's worth noting, escaping the brackets also works: \[some-string\], but we would need to escape any regex character, which may not be feasible in all environments.

When attempting to repro this via a unit test, Click CliRunner does not hit the same issue. This could be a different bug. I had to use subprocess to hit the issue.

Environment:

• OS: Windows 11
• Python version: 3.9.1
• Click version: 8.0.0 and 8.0,3

[interifter]

Another proposed implementation:

Inside an option, expose this:

@click.option(expand_args=False)

to allow explicit bypass of this behavior (I was looking through the code to figure out how to do this, but there is a lot more going on than I thought :))

And inside the function:

glob_arg = arg if expand_args else escape(arg)
matches = glob(glob_arg, recursive=True)

[davidism]

See #1901

[interifter]

So, that is supported?

And should we consider the security vulnerability aspects of this?

[davidism]

Yes, expanding globs on Windows is supported internally, since the Windows shell doesn't do it. #1918 added a way to disable it. Escaping the values before passing them to glob would completely defeat the purpose. I'm unclear how it could be a security issue, the user controls what values they pass to the command on their own machine.

Please review https://github.com/pallets/click/security/policy for how to report security issues. This is not the appropriate venue for that, and you'll need to be much more specific.

[interifter]

Apologies, but it took a while to figure out how to implement #1918.

Is it possible to expand on documentation for it? Or is it already there and I am just bad at finding it? :)

For us, we had to do this:

main = click.CommandCollection(sources=[scripts, plugins])

if __name__ == "__main__":
    sys.exit(main(windows_expand_args=False))

[davidism]

You don't need sys.exit, otherwise it's correct.

[interifter]

Hmm... we're having issues with this fix in another one of our environments.
I doubt it's related to click, but it seems it's ignoring windows_expand_args=False.

Thank you for all the pointers.

[interifter]

Yep. Our code path problem.

So, following up, why wouldn’t we want to expose this at a click option level? That is, be able to suppress/enable as a keyword arg per option?

[davidism]

Because it doesn't happen at the option level, it happens once before parsing sys.argv.

[davidism]

Follow up in #2195, bad patterns will be left as-is instead of causing an error.