3 ms·

This feels like a pretty dramatic response when this is already a problem with child_process.exec and most shell scripting languages in the first place. I’m no

by akst 5y ago

This feels like a pretty dramatic response when this is already a problem with child_process.exec and most shell scripting languages in the first place. I’m not sure what you’re expecting if you’re taking arbitrary IO output to build up a process with arguments separated by spaces. A lot of things had to go wrong before you get to this point. If we’re being realistic here, this library’s likely intended use is for this is smallish scripts anyways… not large pieces of software that are creating commands on the fly to from arbitrary IO

pwdisswordfish8 5y ago
Ordinary variable substitution in shells splits on spaces, which is still bad, but at least doesn’t immediately lead to arbitrary code execution. I’m expecting at the very least an equivalent of Python’s shlex.quote. This is supposed to be an improvement on the status quo, not a regression.
goshx 5y ago
> If we’re being realistic here, this library’s likely intended use is for this is smallish scripts anyways… not large pieces of software that are creating commands on the fly to from arbitrary IO If we're actually being realistic here, we know users will use this for whatever scenario, regardless of the author's intent.
lhorie 5y ago
But with child_process.exec you can at least pass values via env and have the shell script come from a file (which you can throw shellcheck at) Also, spawning node from shell to spawn shell to spawn something like ls is madness. Node has fs.readdir already and there are util packs on NPM like fs-extra and friends.
seniorsassycat 5y ago
You should use child_process.execFile or the execve equiv in your language. Shell has expansion issues but rigorous quoting helped by shellcheck make it safe. And zx's `$` could make better use of tagged template literals. Something like this, tho it isn't correct function $(strings, ...args) { const cmd = []; for (const part of strings) { cmd.push(...part.split(/\s+/)); if (args.length > 0) { cmd.push(args.shift()); } } return child_process.execFile(cmd[0], cmd.slice(1)); } https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#tagged_templates https://developer.mozilla.org/en-US/docs/Web/JavaScript/Refe...

by akst 5y ago

pwdisswordfish8 5y ago

Ordinary variable substitution in shells splits on spaces, which is still bad, but at least doesn’t immediately lead to arbitrary code execution. I’m expecting at the very least an equivalent of Python’s shlex.quote. This is supposed to be an improvement on the status quo, not a regression.

goshx 5y ago

> If we’re being realistic here, this library’s likely intended use is for this is smallish scripts anyways… not large pieces of software that are creating commands on the fly to from arbitrary IO If we're actually being realistic here, we know users will use this for whatever scenario, regardless of the author's intent.

lhorie 5y ago

But with child_process.exec you can at least pass values via env and have the shell script come from a file (which you can throw shellcheck at) Also, spawning node from shell to spawn shell to spawn something like ls is madness. Node has fs.readdir already and there are util packs on NPM like fs-extra and friends.

seniorsassycat 5y ago

You should use child_process.execFile or the execve equiv in your language. Shell has expansion issues but rigorous quoting helped by shellcheck make it safe. And zx's `$` could make better use of tagged template literals. Something like this, tho it isn't correct function $(strings, ...args) { const cmd = []; for (const part of strings) { cmd.push(...part.split(/\s+/)); if (args.length > 0) { cmd.push(args.shift()); } } return child_process.execFile(cmd[0], cmd.slice(1)); } https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#tagged_templates https://developer.mozilla.org/en-US/docs/Web/JavaScript/Refe...