Add `--query` for `fzf_history` and properly escape the command

[pukkandan]

While --query is used for other commands, it is currently disabled for fzf_history with this comment in the code

    -- This intentionally does not use '--query' because that isn't safe:
    -- Depending on what the user has typed so far, passing it as an argument
    -- may cause the command to interpreted differently than expected.
    -- E.g. suppose the user typed:     "pgm.exe & rd /s
    -- Then fzf would be invoked as:    fzf.exe --query""pgm.exe & rd /s"
    -- And since the & is not inside quotes, the 'rd /s' command gets actually
    -- run by mistake!

This is my attempt at properly quoting the command so that such issues do not happen. Quoting the argument for a command is easy. We just need to quote it and escape all literal quotes. The tricky part is that io.popen seems to internally call a cmd /c. So the command needs to be quoted again for cmd's parsing.

The way cmd.exe parses its command line is weird. But from some research, the easiest way to quote it appears to be to wrap the entire thing in quotes and then escape all special characters with ^. |&<>%^! is all the special chars that I know of. I also found some sources that claim ()@ may need to be escaped under certain conditions. So I've added them to the list as well (Escaping characters that don't need it appears to be harmless)

[pukkandan]

hm.. right after I submitted this, I found a bug 😓

"a & b works correctly, but a & b results in a ^& b. Atleast it is safe (ie, no unwanted code execution) 🙃

I will look into it

[chrisant996]

hm.. right after I submitted this, I found a bug 😓

"a & b works correctly, but a & b results in a ^& b. Atleast it is safe (ie, no unwanted code execution) 🙃

I will look into it

Yes, quotes require additional/different quoting. I've never dealt with that in cmd command lines before (or I've forgotten having done it before 🤷🏻‍♂️😄), so I postponed doing it.

Thanks for working on this!

[chrisant996]

|&<>%^! is all the special chars that I know of. I also found some sources that claim ()@ may need to be escaped under certain conditions. So I've added them to the list as well (Escaping characters that don't need it appears to be harmless)

Run cmd /?, and at the very end of the text it lists all the special characters that need quoting. They may have some interplay with escaping, and some of them may also need escaping in order to pass through the quoted command line and reach fzf properly.

[pukkandan]

I assume you were talking about this

The special characters that require quotes are:
     <space>
     &()[]{}^=;!'+,`~

This is referring to the arguments that need quoting when passing to any app. That is not a problem for us. We can unconditionally quote the argument and it works fine. The issue we have is specifically with escaping characters when passing to cmd.exe. cmd.exe uses wildly different quoting rules than the conhost.

If /C or /K is specified, then the remainder of the command line after
the switch is processed as a command line, where the following logic is
used to process quote (") characters:

    1.  If all of the following conditions are met, then quote characters
        on the command line are preserved:

        - no /S switch
        - exactly two quote characters
        - no special characters between the two quote characters,
          where special is one of: &<>()@^|
        - there are one or more whitespace characters between the
          two quote characters
        - the string between the two quote characters is the name
          of an executable file.

    2.  Otherwise, old behavior is to see if the first character is
        a quote character and if so, strip the leading character and
        remove the last quote character on the command line, preserving
        any text after the last quote character.

According to this (point 2), the command line passed to cmd is parsed as-is (including any other quotes) when the entire command is surrounded by double-quotes. The PR already does this.

But in our case, the command is first interpreted by the conhost (I think - this depends on implementation details of io.popen), then by cmd.exe, then again by conhost and then passed to fzf. It's this multiple levels of interpretting that is causing issues.

I will need to experiment a bit to figure out exactly what is going on. It might take me a few days to get back to this.

[chrisant996]

Conhost is not involved. This is purely about how cmd.exe parses the command line passed to it, and there is only one level involved. And, more specifically, the only thing that is problematic is when the text to pass already includes quote character(s).

[chrisant996]

I've looked at this a bit deeper.

There appears to be no way to using quoting or escaping of quote characters themselves to get the text to reach fzf.exe accurately when passing through cmd.exe.

It's possible bypass cmd.exe, but that requires a significant amount of changes to Clink itself, plus some complicated new Lua APIs to spawn multiple processes and pipe results between them and back to Clink. While it's certainly possible to do, that's a whole lot of work and complexity to deal with an uncommon situation. I'm not going to build all that stuff just for this. 🙃

@pukkandan, how about adding a check so that it only does quoting/escaping/--query if the input line has no double-quote characters? And if it has any double-quote characters, then just skip the --query part like it currently does?

[pukkandan]

You are right. I can't figure out a way to pass all cases correctly either 😢

I personally don't tend to look up history for any complex expressions, so I have never had it break in actual use so far. So I will keep using this. But I don't see a way to make this work well enough to be merged

how about adding a check so that it only does quoting/escaping/--query if the input line has no double-quote characters? And if it has any double-quote characters, then just skip the --query part like it currently does?

I think this inconsistency will be confusing to users and is not worth it

[pukkandan]

It's possible bypass cmd.exe, but that requires a significant amount of changes to Clink itself, plus some complicated new Lua APIs to spawn multiple processes and pipe results between them and back to Clink. While it's certainly possible to do, that's a whole lot of work and complexity to deal with an uncommon situation. I'm not going to build all that stuff just for this.

Totally understandable. It is a shame lua has no inbuilt method to bypass shell when spawning processes.

I am mostly a python dev and have been spoiled by the subprocess module 😄

[chrisant996]

Totally understandable. It is a shame lua has no inbuilt method to bypass shell when spawning processes.

Bypassing the shell is easy.

The complexity is in building an alternative piping and redirection system. The fzf command in question involves an additional level of piping. I wouldn't expect Lua to have built-in APIs for something so niche and complicated as that.

[chrisant996]

I personally don't tend to look up history for any complex expressions, so I have never had it break in actual use so far. So I will keep using this. But I don't see a way to make this work well enough to be merged

That is of course up to you.

Do be aware that this is susceptible to what is called an injection attack. Accidental "friendly fire" attacks are a real thing. 😉

There is potential for quoting problems there to run a functionally different command than was intended, and have potential to cause great harm.

It's the same class of issue as making your password "drop table" and unexpectedly altering how a SQL query behaves, and causing it to accidentally delete an entire password table from a database.

Please do be very careful, and good luck!

how about adding a check so that it only does quoting/escaping/--query if the input line has no double-quote characters? And if it has any double-quote characters, then just skip the --query part like it currently does?

I think this inconsistency will be confusing to users and is not worth it

That is why I chose to comment out the code. It is a choice between quite real risk of significant damage to the computer, vs being wonderful 99.9% of the time but once in a while "mysteriously" doing nothing instead of causing damage.

Again, do be aware that the implementation you're using is very dangerous. And who knows who might apply and use your patch without understanding the dangers. Just something to consider.

[pukkandan]

There is potential for quoting problems there to run a functionally different command than was intended, and have potential to cause great harm.

I am aware of this potential issue. But as far as I know, the scheme I am using never causes any command execution. The only issue I have found is that it often unnecessarily escapes certain characters. I honestly don't know if this is guaranteed to be safe though.

If you can think of any string that can cause command execution with this, please let me know

Edit: I did find a way to perform injection with it. So yeah, definitely not safe and use at your own risk

Again, do be aware that the implementation you're using is very dangerous. And who knows who might apply and use your patch without understanding the dangers. Just something to consider.

I don't plan to share this anywhere (except this PR ofc). So anyone who wants to use it hopefully reads this discussion first

[chrisant996]

Cool, thanks. Closing this pull request.