Use hard quotes rather than soft quotes #61902

[geirha]

The current code encloses arguments in soft quotes ("...") before injecting them into the shell. Soft quotes prevent some expansions from occuring, but not all. Hard quotes ('...') on the other hand, treats everything literally, making it a much safer choice.

[geirha]

rebased with master, so now the automated tests finally succeed. I'm unable to properly test this patch locally myself, because the integrated terminal ends up broken in my build attempts.

[weinand]

@geirha Thanks for the PR.

Two comments:

• you modify the behavior of the quote function which is not only used for env variables and the cwd, but for args as well. Your change will break anyone who passes wildchars ('*') as arguments. I suggest to only fix the env variable issue launch.json envFile not escaped before injecting #61902 and not touch anything else.
• The original code tried to add quotes only if needed in order to keep the resulting command readable. Please try to do the same when quoting env vars.

[geirha]

• you modify the behavior of the quote function which is not only used for env variables and the cwd, but for args as well. Your change will break anyone who passes wildchars ('*') as arguments. I suggest to only fix the env variable issue launch.json envFile not escaped before injecting #61902 and not touch anything else.

I feel that'll be a messy, half-working solution, where a glob will only work if it happens to not contain space or ", regardless of how many quotes/backslashes the user throws at it. I'd suggest instead that the glob expansion be done by vscode, and then pass the resulting file lists safely quoted. Though unfortunately there's no builtin glob function(s) in either js or node.js that I know of, but there are some available via npm of course. Not sure what the politics of introducing new deps is..? I see there's a glob in devDependencies already.

• The original code tried to add quotes only if needed in order to keep the resulting command readable. Please try to do the same when quoting env vars.

I thought about adding a whitelist of characters not needing quoting. but went with trying to make the patch small instead. I can add such a whitelist.

[weinand]

The scope of your PR is to address a problem with env variables #61902, not to fix globbing issues (that are not even mentioned in the original comment).
Please keep in mind that there are quite a few launch configurations out there that we don't want to break unnecessarily.

If you want to address globbing issues, please create a separate issue first.

[geirha]

Fair enough. So I reverted the quote function to its previous state, and introduced a new hardQuote function for the cwd and env vars, including a whitelist to avoid it needlessly quoting words that don't contain any shell metacharacters.

[weinand]

Thanks for the PR.
I will limit the use of hardQuote to env variables only in a subsequent commit.

[weinand]

@geirha The change from ';' to '&&' in your PR resulted in regression #68997 (and I as the reviewer did not notice that).
This is another good example why PRs should never contain unnecessary changes.

[geirha]

@weinand Ouch. It never occured to me that this could be used with other than bourne-style shells. Especially since the code in question is inside a case ShellType.bash:

The hard-quoting might also be wrong then; it's safe for bash, and any other bourne-style shells I know of, but I have no idea if fish has the same quoting rules, nor whether the whitelisted characters are safe to leave unquoted.