child_process: add an API to escape shell argument

[PoojaDurgad]

child_process.escapeArgument() escapes shell arguments so that those can be passed to spawned scripts, which expect arguments as escaped strings

Fixes: https://github.com/nodejs/node/issues/34840

Co-Authored-By: Ben Noordhuis [email protected]

Checklist

• [x] make -j4 test (UNIX), or vcbuild test (Windows) passes
• [ ] tests and/or benchmarks are included
• [x] documentation is changed or added
• [x] commit message follows commit guidelines

[mscdex]

nit: Ben's email address seems to be truncated at the end

[mscdex]

This needs some tests.

[Trott]

/ping @nodejs/child_process @nodejs/tsc @devsnek

See conversation in https://github.com/nodejs/node/issues/34840 for arguments for and against including this in core.

[Artoria2e5]

Implementation is incorrect in that POSIX sh and Windows have very different methods. I don't get how you expect to get away without an OS check.

FWIW, the current escape function is overcompilcated but correct for POSIX sh. The tests address the behavior of the escaper only, not how the escaped string is promised to behave.

[gireeshpunathil]

I too support this.

• posix vs windows parity: fixable
• userland vs core: for a user, this is a capability that comes coherent with spawning.

[ChALkeR]

I tend to agree with @vdeturckheim here.

For a specific example, see e.g. https://github.com/feross/cross-zip/pull/18 (and https://github.com/feross/cross-zip/pull/16 for history).

Afaik, there wasn't a good way to escape the argument inside the powershell command there, except for specifying it as a separate argument (as that PR demonstrates). Note how that is happening inside the argument. The correct solution was to not concatenate the argument inside the command.

I'm afraid that a presence of such an API could be abused for escaping arguments like in case above (i.e. inside the powershell command, which would have been incorrect), and do more harm in that case.

I also don't see the benefit of this change yet — users should use the structured API that accepts an array of arguments instead of building a shell string by concatenation, and introducing this will have a negative effect of moving them to a less secure API where it would be easy to miss escaping (even assuming that it works correctly).

Could someone elaborate on the benefit of this change, please? Which usecases need this over the current API (like execFile)?

Upd: left a comment at https://github.com/nodejs/node/issues/34840#issuecomment-724164638 — I don't think that the usecase described there needs this change.

[ChALkeR]

In short: I think that addition of a method for manual escaping will make the issue worse, not better. A better solution would be to discourage using .exec at all in favor of .execFile.

[ChALkeR]

I'll investigate how the the current API/documentation could be improved on this soonish. Likely not today, but this week.

[Artoria2e5]

@ChALkeR

I'll investigate how the the current API/documentation could be improved on this soonish. Likely not today, but this week.

IMO the issue really is that node allows shell: true with an array of args to start with. If there's no shell-calling API then everything is completely unambiguous: what you pass into the array is what the program's main() function will see, save for the case where the programmer must hand-craft a verbatim cmdline for special Windows programs.

But when there is an API that calls programs by launching shell the issue immediately gets complicated. What does an array mean now? Is ['a b'] different from ['a', 'b'] anymore? Node chose to make the two things the same, making platform-dependent escaping a user's problem. To quote your response,

users should use the structured API that accepts an array of arguments instead of building a shell string by concatenation

Node itself is doing it by naive concatenation. There's no structure to start with! If we agree that this is messed up, then Node should at least provide tools for the user to un-mess it. Either in the form of this PR that gives a separate method, or in the form of #29576 that does it in the shared spawn code-path (shell invocation is platform dependent anyways, so why not offer to escape too?).

PS: I hereby authorize PoojaDurgad to use stuff (code, doc, tests) from my PR as they see fit. Windows is a messy beast to tame, and it's better to work together.

[gireeshpunathil]

I think there are 3 topics here:

• strength / fidelity of method that craft the escape sequence (security)
• having this in core versus in userland
• this PR versus other PR(s)

On the first one, chances of exploit should not be quoted as a reason to not to have this, if the chances of exploit is coming from unknown possibilities.

If shells of certain platform(s) do not work properly, those can be clearly documented as caveats. We have done that in hundreds on APIs in the past. Not every API abstracts every platform seamlessly.

On the second one: this is coherent with spawn calls. It is not so great to go to third party to prepare your input and then use core API to make the call. I am not sure we want the core to reduce to mere platform abstractions.

On the third one: I recommend (though no compulsion) @Artoria2e5 and @PoojaDurgad to collaborate, bring-in the best from both, rather than running with two PRs.

[github-actions[bot]]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions[bot]]

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.