add property for controlling escaping arguments for the `runInTerminal` request

[weinand]

Currently it is not possible to control the escaping of the args property on the runInTerminal request.
As a consequence the client (frontend) has to guess how to escape the arguments when passing them to a command shell.
This does not always result in the intended behavior.

This feature request asks for a mechanism to control the escaping.

[weinand]

Today "preserving of characters in args" is not explicitly mentioned in the spec, but client implementations that rely on command line interpreters ("shells") try to do their best to escape many characters from being interpreted by the shell. So the undocumented semantics of the args array in the runInTerminal request is:

"arguments strings are passed to the command unmodified and they are not interpreted by an intermediate command line interpreter".

However, some clients are not that strict and exclude a few characters from escaping, most notably the input/output redirections <, >. With this loophole it is possible to redirect input and output, but on the other hand it is very difficult to pass < and > characters to a command.

In order to cleanup this "fuzzyness" I propose the following:

• the undocumented semantics of the args property of the runInTerminal request will be documented.
• a new optional property dontEscapeArgsForShell will be added to the runInTerminal request. If the property is true, the individual strings of the args array will be passed unescaped to the command line interpreter.
• a new client capability supportsUnescapedArgs that tells debug adapters whether the feature is available.

For debug extension authors this means:

If a debug extension contributes an args array property we recommend that a corresponding optional property dontEscapeArgsForShell is added which gives users a way to control the escaping behavior.
This makes it possible to make the existing "partial escaping" more strict and predictable: now all problematic characters can be escaped, this includes < and >.

@roblourens @connor4312 what do you think? Do you have better naming suggestions for dontEscapeArgsForShell and supportsUnescapedArgs?

[connor4312]

I dislike "negative" boolean flags, perhaps it can be named escapeArgs and supportsEscapeArgs, with sanitization taking place unless it's specified as false?

[puremourning]

I think the simple solution here is just to document that arguments must never be modified or escaped. if the debug adapter wishes to use a command shell interpreter to do anything at all it must put the command interpreter as the effective argv0 (e.g. [ '/bin/sh', '-c', 'the command line here etc.' ]

We shouldn't add flags to indicate whether or not to 'escape for shell' because we haven't got any specification for what a 'shell' is or which shell should/would be used by the client. For example, if the user's default shell is 'powershell', is the client somehow expected to know how to escape commands for that by magic?

Currently, clients are expected, I believe not to use a shell at all and rather run command supplied directly (as if by calling execvp, or CreateProcess, passing the arguments essentially unmodified[1]). Am I missing something here?

In any case, I'm not sure I followed how a client should interpret the new flag if they currently don't inject arbitrary shell commands into the command line. The suggestion, by having the flag, is that somehow clients are expected to be sticking some shell in front of the command and then escaping the arguments, unless this flag is supplied. That seems backwards to me, and means that all of a sudden clients like mine which don't inject shells into commands become nonconforming, despite doing what would ostensibly be the most portable and understandable thing.

[1] - I realise that on windows CreateProcess takes the arguments as a string not an array. Is that the issue here?

[weinand]

@connor4312 yes, I don't like negative flags (e.g "dontX...") too, but I even more dislike a flag semantics where a missing flag means "true". I prefer the "falsy" semantics: "undefined" or "false".

[connor4312]

Maybe then we can call it rawArgs or (not as good) unsanitizedArgs?

@puremourning for context on this, this came up from microsoft/vscode#148887 in which it appears that quite a few users were dependent on VS Code not escaping arguments entirely correctly (7 👍's is a decently high number for debug issues.)

Though I think it is reasonable that the default would be to not escape arguments, VS Code has been escaping them to greater or lesser degree for quite a few years, so ultimately at least one implementation will become non-confirming.

[puremourning]

Unless you specify what escaping rules should be applied so that all clients can implement it, then not only will good-citizen clients du jour become nonconforming, but the specification becomes ruled by de facto implementation which makes it impossible for independent client implementers to actually conform.

[weinand]

@puremourning the issue we are trying to solve is microsoft/vscode#149910 and microsoft/vscode#148887.

The runInTerminal request should run the given commands and arguments without modifying them in any way. If the underlying implementation in the client uses a command line interpreter, the implementation will have to escape some characters in order to pass the arguments unmodified.

I do not see a need why a debug adapter should introduce another command line interpreter. If users want to run the debuggee in the integrated terminal (which they have configured to use PowerShell), why should the DA use "bash" on top of that?

However, some users know what they are doing and they want to pass shell syntax to the underlying command line interpreter running in the terminal. They accept the fact that doing so does not work on all platforms. They want to be able to pass arguments in a "raw" but predictable form, e.g. a "> /tmp/out" to pipe something into a file. They would use the new flag to do so.