Improved shell option

[ehmicky]

Problems

The shell option currently has several shortcomings making it both unsecure and not cross-platform.
(For reference, link to the code in Node.js)

shell: true does not make sense

shell: true runs /bin/sh -c ... on Unix and cmd.exe /d /s /c "..." on Windows.

This means the shell command must work on both Unix shells and cmd.exe. In practice, most of cmd.exe syntax does not work with Unix shells, with very few exceptions (such as &&).

So shell: true is actually used in one of two ways, and neither makes sense:

1. The user actually intends to run a Unix shell on both Unix and Windows. Or does not care about Windows at all. However, that's not what shell: true does on Windows: it runs cmd.exe instead.
2. The command and arguments does not include any shell-specific syntax. But then, there is no reason to use a shell in the first place.

There are potentially some edge cases where this might be useful. For example, npm allows installing different binaries for Unix and Windows, by using different file extensions like *.cmd. Those cross-OS binaries would be designed to run with shell: true, running OS-specific shell code. However, I believe this is not common. Notable examples include Yarn.

shell does not escape arguments

When using the following:

await execa('file', ['one two', 'three'], {shell: true});
await execa({shell: true})`file ${'one two'} ${'three'}`;

Execa actually interprets this as calling file one two three. While the user clearly indicates that the first space should be escaped, it is not. Not only if this against user's intent, it is also a security risk.

The reason is that child_process.spawn() lets users handle their own quoting, when using the shell option. It just concatenates arguments with spaces as delimiters. That's probably because quoting is finicky. Also, the shell option allows any file path to be passed (as opposed to an enum of allowed shells), so it's a little harder for child_process.spawn() to know which shell is being used.

shell can merge arguments

In a similar way, when the shell option is used, some arguments might be merged together. For example:

await execa('file', ['"one', 'two"'], {shell: true});
await execa({shell: true})`file ${'"one'} ${'two"'}`;

Is interpreted as file "one two", i.e. as a single argument instead of two.

Binary lookup

That's a more minor issue, but shell: true uses absolute file path /bin/sh (and /system/bin/sh on Android). It probably should use sh and let the OS does a PATH lookup instead. This would be more cross-platform.

Solutions

Automatic quoting

We could allow specific shells as possible values for the shell option. Like sh, bash, zsh, dash, tcsh, ksh, fish, cmd.exe, powershell. For example:

await execa('file', ['"one', 'two"'], {shell: 'bash'});

Is interpreted as file '"one' 'two"', i.e. two arguments.

All Unix shells might be able to use the same quoting logic (single quotes), so I don't think the quoting logic would be too hard to implement and maintain. Actually, we already use the following logic for result.escapedCommand. We would need to tweak and thoroughly test it due to the security implication, but that's doable.

execa/lib/arguments/escape.js

Lines 62 to 72 in fdd8a9f

	const quoteString = escapedArgument => {
	if (NO_ESCAPE_REGEXP.test(escapedArgument)) {
	return escapedArgument;
	}
	return platform === 'win32'
	? `"${escapedArgument.replaceAll('"', '""')}"`
	: `'${escapedArgument.replaceAll('\'', '\'\\\'\'')}'`;
	};
	const NO_ESCAPE_REGEXP = /^[\w./-]+$/;

Discourage shell: true

We could rename shell: true to shell: 'unknown', to discourage it.

We would rename shell: false to shell: 'none', the default value.

Backward compatibility

The above would be mostly backward compatible:

• Although technically possible, most users aren't probably currently using shell: 'bash'. Even if they were, the above would just probably quote their arguments, which would only be breaking if they rely on arguments being concatenated.
• We would keep shell: boolean in the source code and type. Its behavior would not change. But we would undocument it and deprecate it.

End result

This would result in the following shell option: 'none' | 'sh' | 'bash' | ... | 'cmd.exe' | 'unknown'. By default, there is no shell. If users wants to use a shell, they must pick one. They can still pick 'unknown', but our documentation warns against it.

All options but 'unknown' would be more cross platform, and more secure.

What do you think?

[sindresorhus]

Is it really worth spending a lot of effort on this? Almost every use-cases is better done with shell: false. I think we should rather spend that effort writing recipes on how to solve shell: true use-cases with shell: false.

We are also not doing users any favors by making shell: true easier to use, as they are more likely to make security-related mistakes with shell: true.

I also fear this will create a large maintenance burden and also result in us having to deal with security issues.

---

We could allow specific shells as possible values for the shell option. Like sh, bash, zsh, dash, tcsh, ksh, fish, cmd.exe, powershell. For example:

I would not know what shell the user has installed, which would limit this to just local scripts. And even then, it would mean users cannot easily share their scripts with others or maybe even other devices they own.

[ehmicky]

Alright, sounds good. 👍

I'm working on improving the documentation right now. I'll include more information about avoiding shells, and how the shell option basically prevents proper escaping.

---

Side note: we do agree that shells should almost never be used programmatically, which I strongly believe in too! :)
That's something that distinguishes us from zx (which uses a shell by default) and bun shell (which uses a shell-like DSL). IMHO Shells were designed for interactive input: using them for programmatic scripts was a nice shortcut decades ago, but it does not make much sense today.

In the end, many features provided by Execa are actually similar to features provided by shells: command execution, I/O redirection, piping, background process, signal termination, exit code, etc. But it's done in a programmatic-friendly way, as opposed to using a terminal-entry-like DSL.

[sindresorhus]

IMHO Shells were designed for interactive input: using them for programmatic scripts was a nice shortcut decades ago, but it does not make much sense today.

I'm in full agreement.

[ehmicky]

I was reading the original (very long) issue and the original intent behind shell: true is actually not really to run shell scripts written in both Bash and cmd.exe. Instead, it is to run cmd.exe on Windows because it is required in many situations: PATHEXT environment variable, .cmd files (which are installed by npm), commands with spaces (in some specific cases), and more.

The issue is that shell: true has the side effect of running the command in a shell on Unix too, even when it was only intended to fix the above issues on Windows.

The ideal situation seems to be to run cmd.exe only on Windows and only if meeting one of the issues described above. This is basically what node-cross-spawn (and therefore Execa) is doing. This makes shell: true much less useful with Execa.

I wanted to bring that up, but this does not change this issue's status.

[sindresorhus]

Maybe worth mention in https://github.com/sindresorhus/execa/blob/main/docs/shell.md ? That pretty much the only reason to use shell: true is for that.

[ehmicky]

Good point. Added #997 to follow up on this.