Built-in method to escape shell arguments

[ThisIsMissEm]

Is your feature request related to a problem? Please describe.

Yes, I was using execa which is a light wrapper for childProcess.spawn to execute a script (call it ./scripts/configure) which took user input as an argument. One of my users supplied "Users & Permissions Management" as that input, which caused the script to hang as the resulting spawned process was:

./scripts/configure Users & Permissions Management

I realised as soon as the bug was reported that I should've escaped the string passed into my function that called execa, so then I looked for modules to correctly escape shell arguments, and they seem pretty complex. Which leads to the question: do I really want to depend on a third-party module to correctly escape shell arguments? Am I just trading one security risk for another?

Describe the solution you'd like

Have a method like childProcess.escapeArgument(arg: string): string which correctly escapes the given value such that it is just a string for all terminals (cross-platform).

Clarification: I am not arguing for childProcess.spawn to escape arguments into strings by default, as that'd be a breaking change, even though it would likely be for the best (if you wanna pass multiple arguments, use the array syntax, not a string). Instead, I'm just asking for a method built-in that's well tested to escape an argument into a string argument for a shell command.

Describe alternatives you've considered

Various NPM modules, writing it myself, etc. All just shift the security responsibility to arguably worse places. This seems like due to the security benefits it can give, it'd be a good candidate for being a built-in function, ideally backported to LTS's

[ThisIsMissEm]

There are also some really bad suggestions on StackOverflow about how to do this: https://stackoverflow.com/questions/1779858/how-do-i-escape-a-string-for-a-shell-command-in-node

(my favourite there is "covert it to base64, then pipe base64 -d then pipe to your command")

[ThisIsMissEm]

More background context: https://twitter.com/thisismissem/status/1296071082504531970?s=21

[devsnek]

I think, overall, you just shouldn't be trying to pass user input into shell commands. if you use child_process.spawn directly you can pass arguments as an array without worrying about escaping but you're still passing arbitrary input into exec.

[ThisIsMissEm]

the problem is even with child_process.spawn you'll still end up with incorrect command execution.

const value = "Users & Permissions Management";
child_process.spawn('./scripts/configure', [value])

Will still result in ./scripts/configure Users & Permissions Management, and you do sometimes need to be able to accept user input and safely pass it to another command.

[devsnek]

@ThisIsMissEm that would only be a problem if ./scripts/configure is a shell script. every shell has different rules for how arguments are parsed, so i'm not sure there's a productive path forward to us solving that in node.

[ThisIsMissEm]

Existing Modules:

• shell-escape
• shell-quote
• sqh

Each of these produces different output. In ruby, the shellwords.escape method, which is commonly recommended also produces different output to all of those, iirc.

[ThisIsMissEm]

In my specific case, it's ('yarn', ['configure', '--set-something', value]) and the script under yarn configure actually also executes a child process, so it's at least two child process executions away from the arguments parser

But if I could easily escape value such that is was a string and always a single string, then the value would pass through fine.

[ThisIsMissEm]

@devsnek this is purely about providing a way to escape a string for shell usage — I would imagine majority are similar, maybe some differences between windows and unix/posix compliant shells, but that'd be it, I'd imagine.

[devsnek]

userland is the appropriate place for this imo.

[bnoordhuis]

Just so we're on the same page, the algorithm for escaping one shell argument is this:

function quote(s) {
  if (s === '') return `''`;
  if (!/[^%+,-.\/:=@_0-9A-Za-z]/.test(s)) return s;
  return `'` + s.replace(/'/g, `'"'`) + `'`;
}

The two if guards are not strictly necessary but they make for nicer output. This one-liner is also valid:

const quote = (s) => `'` + s.replace(/'/g, `'"'`) + `'`;

I'm a bit on the fence as to whether something that simple belongs in core but I wouldn't strenuously object.

[silverwind]

Another alternative is to replace with escaped single quote. Not sure if it'd make any practical difference.

s.replace(/'/gm, `'\\''`)

[bnoordhuis]

https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#Single-Quotes

Enclosing characters in single quotes (‘'’) preserves the literal value of each character within the quotes. A single quote may not occur between single quotes, even when preceded by a backslash.

[silverwind]

Of course the output would be wrapped in single quotes so it's not actually putting single quotes inside single quotes. I consider it more secure because it keep variables inside single quotes inside such while the "'" method enables variable expansion (which may expose secret environment variables to the user):

$ bash -c 'echo "'"$EDITOR"'"'
nvim
$ bash -c 'echo '\''$EDITOR'\'''
$EDITOR

I guess this difference alone means there can not be one single escaping method because variable expansion may or may not be desired depending on use case.