Add a way to prevent shell-expansion on commands (this issue is not for exec)

[nfischer]

Once #343 is resolved, we could run into code that has unintended results. For example:

> echo(ls()); // start out in a directory containing these 3 files
[ 'a.txt', 'b.txt', '*.txt' ]
> ls().forEach(function (file) {
  if (file === 'a.txt') return; // don't delete a.txt
  rm(file); // delete the other two fies
});
> echo(ls()); // when we deleted *.txt, it was treated as a glob, so we deleted everything
[]

If we allow shellEscape() to be a function, then we could replace the questionable line with rm(shellEscape(file)), which is guaranteed to not glob.

An alternative would be to insist on the syntax: set('-f'); rm(file); set('+f') (very verbose, but if #344 is resolved, should be safe).

Another alternative would be rm(file, {glob: false}). This is a nice syntax, but complicates parsing. The advantage of this would be that we could extend it to support more than just glob, like silent: true (to emulate echo foo >/dev/null), instead of the very verbose config.silent = true; ls(); config.silent = false, for getting a single command to be silent.

[nfischer]

#344 #343 and this one are all related, and I think it would be a mistake to resolve one without resolving the others. These are also related to #103 and #143, which are more targeted at exec().

[TimothyGu]

Escaping is simply the wrong way to approach this. Ever since C's system() call, it has been empirically proven that no escaping can fully and completely prevent command injection.

Node.js's exec is simply a new iteration of the system() function. It is not only unsafe, but also inconvenient at times. True, there are modules that aim to fix this issue, but as history has proven it is ultimately unsafe to do anything with a shell.

Therefore, I would much prefer if, instead of providing an escape function, shelljs can support using an array of arguments that will be passed to spawnSync or execFileSync. In this case, people know that it is going to be safe. The unsafe original semantics are also preserved in case the user wants to do something simple.

[nfischer]

@TimothyGu this issue isn't about exec. This is specifically about all the other options. #103 and #143 are more pointed at exec specifically

Also, "shellEscape" is just a name. I don't intend on modifying strings when I fix this. One thought is to return an object containing that string value. Then instead of globbing in common.wrap(), we would just convert to the regular string and not glob, so special characters don't have special meanings. Or the "glob: false" option I suggested would be another good (perhaps better) choice. If you have input on either of those ideas, that'd be great 👍

I agree that exec() needs a better solution, and I'm leaning toward something like execFile() under the hood

[ariporad]

@nfischer: I've got an idea for an API exec-wise:

What if we did this (using ES6 string tags):

$`mycommand --opt ${unsafeVar}`

That gets transpiled to:

$('mycommand --opt ', unsafeVar);

Which is a totally acceptable syntax for <ES6.

[nfischer]

@ariporad I updated the title to make the intended topic of this issue clearer. #103 or #143 would be a better spot for your comment I think. Although it looks to me like it would have some issues with multi-word commands, right?

[nfischer]

Closing this, since set('-f') is suitable for this. If we want to add a nicer API (like an options parameter as the last argument), we can open that as a separate issue.