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.

[weinand]

BTW, I still like the approach to use a new commandline property as an alternative to combining the args array with a rawArgs flag to achieve the same.
So instead of:

"args": [ "<", "in.txt", ">", "out.txt" ],
"rawArgs": true

you could say:

"commandLine": "< in.txt > out.txt"

(and this would help in cases where VS Code's variable substitution is used to pass a full command line).

[int19h]

This makes the most sense to me: as a user, either you let the tooling handle all the quoting and escaping, or you're in full control but also responsible for all of it.

[roblourens]

Currently, clients are expected, I believe not to use a shell at all and rather run command supplied directly

We are talking about the runInTerminal request which I think implies that the command is run in some shell and not spawned directly. When not start your debuggee in a terminal, it might be spawned directly by the debug adapter. To be clear just wondering whether you interpreted runInTerminal differently or missed that context. @puremourning

I agree that the client is best suited to escape for a particular shell, being the thing that is starting the shell and sending text into it.

The commandLine looks nice, and makes the difference extra clear, but I like that I can set up my args for normal non-terminal debugging, then set "console": "integratedTerminal", and the program with the same args is started in a terminal. If we use a different launch config property entirely then I have to partially rewrite my launch config.

[roblourens]

Oh and also if I want to pass a real space in an arg with commandLine, now I have to figure out how to escape it in my launch config 😁

[roblourens]

Sorry for the stream of consciousness but actually you are saying that I would continue to use args to get escaped args, but switch to commandLine to pass unescaped text?

[weinand]

@roblourens you asked:

... you are saying that I would continue to use args to get escaped args, but switch to commandLine to pass unescaped text?

yes, that would be my second proposal.

[int19h]

Does it actually need a separate property though? Perhaps just allow "args" to be a string rather than an array? It feels like it'd make more sense from the naming perspective, at least - "commandLine" sounds like the entirety of the command line, but in practice you'd still have the actual run target specified separately.

[weinand]

@int19h

Perhaps just allow "args" to be a string rather than an array?

We seem to mix two things here: on one side we have the DAP spec for runInTerminal and on the other side we have individual debug extensions that can introduce whatever end-user visible properties they want.

In case of DAP I suggest not to allow a single string for args instead of an array because that will be more difficult to support in a non-breaking way.
And for the end-user visible properties we can only make recommendations, but we cannot enforce a style.

[puremourning]

Currently, clients are expected, I believe not to use a shell at all and rather run command supplied directly

We are talking about the runInTerminal request which I think implies that the command is run in some shell and not spawned directly. When not start your debuggee in a terminal, it might be spawned directly by the debug adapter. To be clear just wondering whether you interpreted runInTerminal differently or missed that context. @puremourning

I know it's about run in terminal. But that does NOT imply a shell. A pty is not a shell.

I agree that the client is best suited to escape for a particular shell, being the thing that is starting the shell and sending text into it.

I don't think the client should spawn a command shell at all.

The commandLine looks nice, and makes the difference extra clear, but I like that I can set up my args for normal non-terminal debugging, then set "console": "integratedTerminal", and the program with the same args is started in a terminal. If we use a different launch config property entirely then I have to partially rewrite my launch config.

[weinand]

runInTerminal is used to start a debuggee in a new or existing interactive terminal. That terminal uses a command line interpreter (aka "shell").
Running a command in that interpreter requires the use of the correct syntax which depends on the shell used.

[puremourning]

This optional request is sent from the debug adapter to the client to run a command in a terminal.

This is typically used to launch the debuggee in a terminal provided by the client.

Nothing here implies a command interpreter is required. It states it's for running the debuggee, not a shell. A terminal is not a shell.

To be clear there is no need to run a shell to run a program. Windows explorer is a shell, bash is a shell. these call system calls (execpe / createprocess) that create processes and attach them to pseudo terminals. Given the above definition it's perfectly reasonable and sensible to launch the requested image whiteout using and intermediary (and pointless) command interpreter.

[weinand]

Yes, a shell is an implementation detail of runInTerminal.
The spec for runInTerminal does not mention "shell" anywhere.

However, when a user creates a debug config for launching the debuggee in VS Code's integrated terminal, then users are very well aware of the fact what shell is running in the integrated terminal.
Now they want to use shell features in the debug config.

We are not trying to solve problems for the case where no "pointless command interpreter" is used.
We are trying to solve problems for the case where "command interpreter" is used because the user wants to use it.

[puremourning]

This makes the most sense to me: as a user, either you let the tooling handle all the quoting and escaping, or you're in full control but also responsible for all of it.

and if you're in full control of it, you can be the one to instruct a shell to be executed, i.e,:

• /bin/bash -c 'my command here > output.log'
• C:\windows\cmd.exe /c my command here > output.log
• etc.

[weinand]

@puremourning

This makes the most sense to me: as a user, either you let the tooling handle all the quoting and escaping, or you're in full control but also responsible for all of it.

That's basically one of the proposals here. What problems do you see with that approach?

[int19h]

I think the contention here is that, for clients that implement "runInTerminal" by doing exec() directly, the semantics of "commandLine" is unclear since you're supposed to provide an array, but there are no clear rules according to which the command line is to be split.

But then I'd expect such clients to not support "commandLine" at all. It would still be a conditional feature with a client feature flag, right?

[puremourning]

However, when a user creates a debug config for launching the debuggee in VS Code's integrated terminal, then users are very well aware of the fact what shell is running in the integrated terminal.

Yes, a shell is an implementation detail of runInTerminal.

It's an implementation detail of VSCode not a requirement of the protocol.

We are trying to solve problems for the case where "command interpreter" is used because the user wants to use it.

If the user wan't to use it, they can be the one to instruct it, as above.

Look, I get it. I realise the dilemma is that vscode behaved one way, arguably out of the obvious interpretation of the protocol, people started to rely on that, vscode changed to be more like the protocol interpretation and some users got broken. Now we're looking for a way to unbreak them by giving them a dial to turn to "I know what I'm doing".

My major concern is that we introduce something into the protocol that requires clients to start working like vscode, by exposing options which debug adapters rely on and are extremely difficult and complex to implement well. (and are naturally only ever tested in vscode, and usually ignoring client capabilities). Today all we have to do is execute the command given directly by creating a process and attaching it to a Pty. But tomorrow we might have to start inventing command lines (like /bin/bash -c ... and encoding shell escaping rules into the client for every possible shell).

Let's take a practical example - theoretical, but likely.

Today:

• Adapter MYADAPTER documents its launch arguments with program and args
• vscode users assume the client runs something like /bin/bash -c '$program $args' in a pty and they set args to something like [ '>', 'output.txt' ]
• non-vscode users reading the documentation of MYADAPTER don't because they know their client launches the command "directly" in the terminal

Dystopian Future:

• we add this new "don't escape" option to the runInTerminal request
• MYADAPTER updates its launch configuration (and documentation) to add a dontEscapeProgramArgs option
• vscode users above update their config to set that and they are unbroken \o/
• non-vscode users read the documentation and go "oooh I can include shell commands in my args !!". They try this and it doesn't work - they raise a ticket on their client, who has to explain that this option is vscode-specific and try and explain the nuance of this thread in their extremely limited free time.

@int19h re commandLine I actually have no issue with the debug adapter being able to specify the command to execute within a shell including the shell. It already can do that with a runInTerminal command like this:

"args": [ "/bin/bash", "-c", "command or whatever > output.txt" ]

Adapters can expose 'commandLine' to the user via their launch arguments if they wish.

[puremourning]

I think the contention here is that, for clients that implement "runInTerminal" by doing exec() directly, the semantics of "commandLine" is unclear since you're supposed to provide an array, but there are no clear rules according to which the command line is to be split.

But then I'd expect such clients to not support "commandLine" at all. It would still be a conditional feature with a client feature flag, right?

I'm just not sure what the difference between args and commandLine would be? if using commandLine would clients be required to start in a command interpreter (of the client's choice!?) and wrap and quote/escape (or not), the commandLine

[weinand]

runInTerminal is not a general mechanism to launch programs.
It is a mechanism to launch programs in a "terminal", or to be more precise in the client's "integrated" terminal or in an "external" terminal.
If your client does not support these types of terminals, then there is no need to implement the request.
An implementation based on "doing exec() directly" does not seem to implement the spec.

So what terminals out there do not use command interpreters?

[puremourning]

So what terminals out there do not use command interpreters?

I feel like that's conflating the idea of a 'terminal' - a pty - and a command interpreter - a shell.

• A shell is a UI for launching applications, such as Windows explorer, bash, csh, mc, etc.
• A terminal (pty, console, whatever) is something which displays and connects the I/O communication channels to a running executable (stdin/stdout). In practice, if you like, xterm is a "terminal" and "bash" is a shell/command interpreter. Xterm can run things other than bash - you can run xterm -e "/bin/echo hello" - no "shell" is launched.

When you double click on a console-mode .exe in windows, it doesn't start cmd.exe and ask it to launch the .exe, it launches the .exe and displays its stdout (and attaches its stdin) to a console (aka terminal).

Ditto in non-windows, you can create a pty and execute a command directly in it. That's what happens when you launch a terminal application from a shell, or when you launch a terminal emulator with a specified command (like xterm above) - which is effectively what vim's :term does.

In case it's not clear, the client (vim in this case, but could be anything) is doing the job of the "shell" - launching the application and attaching its standard streams, then displaying the console and providing interactivity. This is exactly what the terminal window in vscode does, but for some reason vscode executes a command interpreter in its terminal, then asks that to run the provided args.

An implementation based on "doing exec() directly" does not seem to implement the spec.

I naturally disagree on that point. It's creating a pty and exec'ing the requested command with its stdout and stdin attached to the pty, exactly like a shell would

to put it another way, if a client implemented the "external console" by doing effectively doing xterm -e "$args" (ignoring, for now that this has the same quoting challenge), would that also not implement the spec, despite ostensibly launching the requested command in a "terminal" (emulator)

[weinand]

I don't get why you would want to go through all the hassles of using runInTerminal if the result is not what you need? If using exec*() directly is what you want, then don't use runInTerminal.
BTW, the default way to launch debuggees (without a shell!) is not handled by runInTerminal at all, but is completely left to the debug adapter (e.g. by using spawn() or fork/exec).

The purpose of runInTerminal is to launch debuggees in the same way (and terminal) as if the user would have typed the command on the command line (including shell characters). So the command history would show the command to launch the debugger and moving through the command history gives users a way to modify arguments etc.

[roblourens]

Whether or not terminal implies shell - I get the distinction and now understand where you are coming from - I think this discussion shows exactly why we didn't want to allow shell characters in the first place. When you create a launch config that includes shell characters, now that launch config is tied to a particular shell. The user is responsible for configuring this that based on their knowledge of the client's shell support and personal configuration. This is awkward but I feel that it's the tradeoff that we have accepted in deciding to support this use case.

If they write powershell in args, and the client does not spawn a powershell shell, it's not the client's failure to respect the DAP spec, it's the user's issue for misconfiguring their launch config.

So then it follows that if someone implements a DAP client that doesn't spawn any shell for a runInTerminal request, it's still the user's responsibility to set up their launch config to account for this.

However if I was just reading the spec from scratch, I would have assumed that the purpose of the runInTerminal request is what @weinand describes, to run the command in a shell.

@weinand also suggested that this new flag would only be enabled by a new client capability flag

a new client capability supportsUnescapedArgs that tells debug adapters whether the feature is available.

[puremourning]

I guess this sort of got a little off the original topic, and I suppose that's probably my fault. I suppose we can put aside whether or not it's correct to launch the app as specified, or first launch a shell, and ask the shell to launch the app, and accept that both are reasonable interpretations and behaviours:

1. We've agreed that by default the arguments should be used verbatim, and therefore if a client elects to start a shell, they are responsible for the quoting and escaping of the command line. (@weinand wrote "arguments strings are passed to the command unmodified and they are not interpreted by an intermediate command line interpreter".)
2. We also agree that it's undesirable for users of such clients who are relying on it not doing that to be broken by doing this.
3. I hope we can at least agree that both approaches (using a shell, or not) are valid interpretations of the specification.

Given that, I guess the proposal is workable, if a little difficult to explain. In the interests of contributing something positive to this, I'd suggest some wording something like:

---

Launching the application

Client implementations are free to launch the application however they choose including issuing the command to a command line interpreter. However the command line in args (including its argument strings) are passed to the command unmodified and they are not interpreted by an intermediate command line interpreter. As a result, clients which use a command line interpreter are responsible for escaping any special shell characters in the args string.

Some users may wish to take advantage of shell processing in the args string. For clients which implement runInTerminal using an intermediary command line interpreter, the argsCanBeInterpretedByShell (TBD: bikeshed about name of field) item can be set to true. In this case the client is requested not to escape any special shell characters in the args strings.

Specifically the meaning if argsCanBeInterpretedByShell: true is:

1. If the client does not use an intermediary shell to launch the application, it has no effect.
2. If the client uses an intermediary shell to launch the application, then the client must not attempt to escape shell special characters, but may escape quotes or other characters as necessary in order to pass the command to the interpreter successfully.

---

Thoughts?

If we define it that way, I don't see the need for a client capability. The point is that what can a debug adapter do if the client does't have the capability and the user requested it? Just fail?

[weinand]

@puremourning thanks for putting my proposal into proper words.

If we define it that way, I don't see the need for a client capability. The point is that what can a debug adapter do if the client does't have the capability and the user requested it? Just fail?

If we had a client capability then a debug adapter could post an alert and warn the user that shell syntax doesn't work because no intermediate shell is used.

---

Here is another version of the proposal:

Launching the debuggee from the debug adapter

Debug adapters are free to launch the debuggee however they choose. Typically the debuggee is launched as a child process and its output channels are connected to a client's debug console via DAP output events. If a debug adapter wants to launch the debuggee in a terminal that is integrated into the client or in a terminal that runs outside of the client (but still configured and managed from the client), it can use DAP's runInTerminal request to use those terminals.

Implementing runInTerminal in the client

Client implementations of runInTerminal are free to run the command however they choose including issuing the command to a command line interpreter (aka 'shell'). Argument strings passed to the runInTerminal request must arrive unmodified (verbatim) in the command to be run. As a consequence, clients which use a shell are responsible for escaping any special shell characters in the argument strings to prevent them from being interpreted by the shell.

Some users may wish to take advantage of shell processing in the args string. For clients which implement runInTerminal using an intermediary shell, the argsCanBeInterpretedByShell property can be set to true. In this case the client is requested not to escape any special shell characters in the argument strings.

Specifically the meaning if argsCanBeInterpretedByShell: true is:

• If the client does not use an intermediary shell to launch the application, it has no effect.
• If the client uses an intermediary shell to launch the application, then the client must not attempt to escape shell special characters [I'm not sure about the: "but may escape quotes or other characters as necessary in order to pass the command to the interpreter successfully." I'd prefer that the client does not escape at all, but I don't know whether this is possible.]

[puremourning]

Typically the debuggee is launched as a child process and its output channels are connected to a client's debug console via DAP output events

How about adding:

Typically the debuggee is launched as a child process and its output channels are connected to a client's debug console via DAP output events. However, this has certain limitations, such as not being able to write to the terminal device directly and not being able to accept standard input. For those cases, launching the debuggee in a terminal is preferable.

Regarding:

"but may escape quotes or other characters as necessary in order to pass the command to the interpreter successfully." I'd prefer that the client does not escape at all, but I don't know whether this is possible

I guess we have to allow it. If the client launches the app with /bin/csh -c "$cmd" or /bin/bash -c "$cmd" or xterm -e "$cmd" , then they must be allowed to esape " characters, else it wouldn't be possible to use them at all... or are you thinking that that's also the responsibility of the user configuring the arguments in their launch config ?

[weinand]

@puremourning thanks for the improved wording. I'll create a new version...

... or are you thinking that that's also the responsibility of the user configuring the arguments in their launch config ?

Yes, that was my thinking. Leaving all the escaping to the user is less confusing than having a mix.

[puremourning]

@puremourning thanks for the improved wording. I'll create a new version...

... or are you thinking that that's also the responsibility of the user configuring the arguments in their launch config ?

Yes, that was my thinking. Leaving all the escaping to the user is less confusing than having a mix.

I fear that will even further bind your users to your internal implementation and the internal implementation details of the debug adapters, but as this won't actually affect me, I don't have "skin in the game" as they say :D

[weinand]

@roblourens @connor4312 @puremourning @int19h here is a new proposal. Let me know if this works for you or whether another round is needed...

---

Launching the debuggee from the debug adapter

Debug adapters are free to launch the debuggee however they choose. Typically the debuggee is launched as a child process and its output channels are connected to a client's debug console via DAP output events. However, this has certain limitations, such as not being able to write to the terminal device directly and not being able to accept standard input. For those cases, launching the debuggee in a terminal is preferable. If a debug adapter wants to launch the debuggee in a terminal that is integrated into the client or in a terminal that runs outside of the client (but still configured and managed from the client), it can use DAP's runInTerminal request to use those terminals.

Implementing runInTerminal in the client

Client implementations of runInTerminal are free to run the command however they choose including issuing the command to a command line interpreter (aka 'shell'). Argument strings passed to the runInTerminal request must arrive verbatim in the command to be run. As a consequence, clients which use a shell are responsible for escaping any special shell characters in the argument strings to prevent them from being interpreted (and modified) by the shell.

Some users may wish to take advantage of shell processing in the argument strings. For clients which implement runInTerminal using an intermediary shell, the argsCanBeInterpretedByShell property can be set to true. In this case the client is requested not to escape any special shell characters in the argument strings.

Specifically the meaning of "argsCanBeInterpretedByShell": true is:

• If the client does not use an intermediary shell to launch the application, it has no effect.
• If the client uses an intermediary shell to launch the application, then the client must not attempt to escape shell special characters. And the user is fully responsible to escape as needed and that arguments using shell characters are no longer portable across shells.

[roblourens]

Sorry, I lost track of what we are proposing right now, is all of the above text to go into the RunInTerminalRequest description? It seems like more detail than needed for the request.

And are we settling on not including a client capability? It seems like the behavior would be better to have a client capability and let the debug adapter decide what to do, rather than sort of fail silently.

[weinand]

@roblourens the description from above tries to explain the rational behind this new property. The fact that we have had this rather lengthy discussion is evidence that the existing specification is inadequate.

Whether we will fold the full description into the runInTerminal request is a valid concern and I will see what I can do to make it shorter. For now let's use the description just as the spec of the new feature.

Client capability:
Yes, I agree, having a client capability (supportsArgsCanBeInterpretedByShell) makes it possible to show a warning if a launch config uses the flag when it is not supported.

[roblourens]

In #146 (comment), I think the first paragraph could be worked into the overview page for DAP, in "Launching and Attaching", if we need more context on why an adapter would use runInTerminal at all.

The second paragraph makes sense in the description for runInTerminal, with some shorter description on argsCanBeInterpretedByShell itself. Working on this.

[weinand]

@roblourens great, thanks!

[roblourens]

Sent this proposal, take a look: #305