Extension API for shell quoting & escaping

[int19h]

VSCode has an implementation of shell quoting & escaping for multiple shells in its implementation of the "runInTerminal" DAP request:

vscode/src/vs/workbench/contrib/debug/node/terminals.ts

Lines 79 to 198 in bd7746b

	export function prepareCommand(shell: string, args: string[], cwd?: string, env?: { [key: string]: string | null; }): string {
	shell = shell.trim().toLowerCase();
	// try to determine the shell type
	let shellType;
	if (shell.indexOf('powershell') >= 0 || shell.indexOf('pwsh') >= 0) {
	shellType = ShellType.powershell;
	} else if (shell.indexOf('cmd.exe') >= 0) {
	shellType = ShellType.cmd;
	} else if (shell.indexOf('bash') >= 0) {
	shellType = ShellType.bash;
	} else if (platform.isWindows) {
	shellType = ShellType.cmd; // pick a good default for Windows
	} else {
	shellType = ShellType.bash; // pick a good default for anything else
	}
	let quote: (s: string) => string;
	// begin command with a space to avoid polluting shell history
	let command = ' ';
	switch (shellType) {
	case ShellType.powershell:
	quote = (s: string) => {
	s = s.replace(/\'/g, '\'\'');
	if (s.length > 0 && s.charAt(s.length - 1) === '\\') {
	return `'${s}\\'`;
	}
	return `'${s}'`;
	};
	if (cwd) {
	command += `cd '${cwd}'; `;
	}
	if (env) {
	for (let key in env) {
	const value = env[key];
	if (value === null) {
	command += `Remove-Item env:${key}; `;
	} else {
	command += `\${env:${key}}='${value}'; `;
	}
	}
	}
	if (args.length > 0) {
	const cmd = quote(args.shift()!);
	command += (cmd[0] === '\'') ? `& ${cmd} ` : `${cmd} `;
	for (let a of args) {
	command += `${quote(a)} `;
	}
	}
	break;
	case ShellType.cmd:
	quote = (s: string) => {
	s = s.replace(/\"/g, '""');
	return (s.indexOf(' ') >= 0 || s.indexOf('"') >= 0 || s.length === 0) ? `"${s}"` : s;
	};
	if (cwd) {
	command += `cd ${quote(cwd)} && `;
	}
	if (env) {
	command += 'cmd /C "';
	for (let key in env) {
	let value = env[key];
	if (value === null) {
	command += `set "${key}=" && `;
	} else {
	value = value.replace(/[\^\&\|\<\>]/g, s => `^${s}`);
	command += `set "${key}=${value}" && `;
	}
	}
	}
	for (let a of args) {
	command += `${quote(a)} `;
	}
	if (env) {
	command += '"';
	}
	break;
	case ShellType.bash:
	quote = (s: string) => {
	s = s.replace(/(["'\\\$])/g, '\\$1');
	return (s.indexOf(' ') >= 0 || s.indexOf(';') >= 0 || s.length === 0) ? `"${s}"` : s;
	};
	const hardQuote = (s: string) => {
	return /[^\w@%\/+=,.:^-]/.test(s) ? `'${s.replace(/'/g, '\'\\\'\'')}'` : s;
	};
	if (cwd) {
	command += `cd ${quote(cwd)} ; `;
	}
	if (env) {
	command += '/usr/bin/env';
	for (let key in env) {
	const value = env[key];
	if (value === null) {
	command += ` -u ${hardQuote(key)}`;
	} else {
	command += ` ${hardQuote(`${key}=${value}`)}`;
	}
	}
	command += ' ';
	}
	for (let a of args) {
	command += `${quote(a)} `;
	}
	break;
	}
	return command;
	}

vscode/src/vs/workbench/api/node/extHostDebugService.ts

Lines 113 to 114 in bd7746b

	const command = prepareCommand(shell, args.args, cwdForPrepareCommand, args.env);
	terminal.sendText(command, true);

Python extension for VSCode has to do very similar with terminal.sendText() a lot, in order to activate environments and to run external tools. Consequently, it also needs to quote & escape the commands that it generates. However, there's no way to reuse the VSCode implementation, because it is not exposed directly as a public extension API, but only via "runInTerminal".

The request is to have this functionality exposed as a public API. Given the shell type and separated command arguments, it should produce a command string suitable for submission to terminal.sendText() or child_process.exec(). The most straightforward implementation would be to publish prepareCommand() as is, since it already provides everything that's needed.

[Tyriar]

Here's what I'm thinking for this issue:

It's a bit of an awkward function to put in the API as I'm not sure where it would go as is. Provided we don't need the flexibility of returning just a string, I think it would be better if we rolled this into a new API on Terminal similar to this:

interface Terminal {
  /**
   * Sends a command to the terminal that will run a shell within the terminal. Using this is it
   * possible to run arbitrary commands, for example using `bash -c` to launch some program.
   * 
   * Note that this is a convenience method similar to `sendText`, if there is no prompt active
   * within the terminal it may not actually do anything.
   */
  sendCommand(options: TerminalCommandOptions): void;
}

interface TerminalCommandOptions {
  /**
   * The shell path to use for the command.
   */
  shellPath: string;
  /**
   * The shell args to use for the command.
   */
  shellArgs: string[];
  /**
   * The directory to run the command under.
   */
  cwd: string;
  /**
   * A map of the environment keys for the command. When values are set to null the environment
   * value will be unset.
   */
  env: { [key: string]: string | null };
}

This seems like the preferable route than having some random function that returns a string to be used within Terminal.sendText.

[int19h]

This is definitely preferable for use in the terminal. But there are some cases where we might want to spawn a background process using a shell (e.g. because environment activation involves shell hooks), without surfacing it to the user in the terminal. Since the underlying implementation for the terminal is going to build a string anyway, it would be nice to have direct access to that in addition to any higher-level terminal API.

[Tyriar]

@int19h understood, do you have ideas on where in the API you would expect to find a function like this?

[int19h]

Not sure there are any existing namespaces that are a good fit. Maybe env, since it already has shell, and shell quoting is kinda sorta about interacting with the environment?

[Tyriar]

Yeah that's what I was thinking too that there isn't really a good fit.

But there are some cases where we might want to spawn a background process using a shell (e.g. because environment activation involves shell hooks)

Would sendCommand in a terminal that uses hideFromUser be appropriate here? You will get escape sequences in output is the main thing here as opposed to when you launch it via child_process.

[int19h]

These are usually very short-running scripts that process argv and/or stdin, and write the result to stdout. Mostly it's JSON, but not always; so escape sequence processing might potentially garble some bits that matter, although I can't think of any specific examples off the top of my head.

My other concern would be perf. We end up having to spawn a lot of processes for things like environment discovery, for example, and it's already a bottleneck. Some of them are redundant or could be reused, but mostly it's unavoidable, and spawning processes in general is pretty expensive. So if the overhead of using a hidden terminal would be significant, it would be a further complication to an already-stressed perf area.

[ewanharris]

For extensions that provide a CustomExecution task and handle their own spawning, a shell quoting API would be a really beneficial API to have exposed rather than only having it available on the Terminal interface

[Tyriar]

@int19h in that case the 2 ways forward are to either find somewhere to put such an API, or not do it and expect extension authors to reimplement it/publish their own shared npm package. Next steps I'd say would be for someone on our team to bring this up in our API call.

@ewanharris I might be misunderstanding but I'm not sure this particular quoting would be useful there? To clarify, this issue about creating and properly quoting a string that would run inside a shell, Quoting to create a command line string might be more appropriate there like this? https://github.com/microsoft/node-pty/blob/0bac8aa107bdde8a25f60ab4d23863a63f249632/src/windowsPtyAgent.ts#L243-L296

[int19h]

@Tyriar We already re-implement this logic, and in our experience, this results in a tail of obscure bugs from corner cases of various shells, that are often already fixed in the VSCode implementation, because it seems more use overall, and thus usually hits them first. Worse yet, it ends up being inconsistent in terms of those corner cases between the debugger (because it uses "runInTerminal") and everything else.

Having the underlying quoting implementation as a separate npm package rather than a VSCode API makes sense to me, but wouldn't it be better to take the one in VSCode, and publish that as a package, for all extensions to use?

[Tyriar]

Going to close this as out of scope as there isn't really a nice place to put a general quoting mechanism in the API. As for an npm package you're free to publish one but moving something out of core would just cause a hassle for us and the code rarely changes anyway. Just continuing to maintain a copy in your extension seems fine too.