feat: new alternative to exec

[nfischer]

Update July 2019

Notice: this is not a security vulnerability, this is a prototype implementation for a feature request. Please see #945 for more context regarding security advisories.

Original message

Add a new command to launch external shell commands, providing a less vulnerable
alternative to exec(), with cross-platform globbing.

I'd love community input on the design of the new API, if this is an agreeable name, etc. This implementation would work well with shelljs-exec-proxy (see nfischer/shelljs-exec-proxy#2), which might be a better API alternative for Node v6+ users.

I believe this, in conjunction with set('-f'), should solve all the relevant security flaws surrounding command injection and unexpected globbing. Globbing is enabled by default in this implementation, for the sake of providing a consistent API across all commands.

This relies on a JS-based implementation of globbing, which is an improvement on exec(), which inconsistently provides globbing on unix but not on windows.

Fixes #143
Fixes #495
Fixes #103

I believe this should also help with #175, #5, shelljs/shx#68, and probably some others.

[nfischer]

@ariporad Please go ahead and start reviewing. The fixes for the test failures should be pretty trivial (some of them are with the tests themselves). This way I can knock out all the fixes at once.

[ariporad]

LGTM!

This is really great, I think that we should consider building a new exec on top of it.

[nfischer]

I'll see if I can figure out why this is failing on unix. This will never pass on v0.10, so I think it's time to remove that from Travis (the version already wasn't supported by us).

This is currently blocked by #525 #523 #522 and shelljs/shx#81, so help with those issues would be appreciated.

[detailyang]

Hi, @nfischer.

what's time your PR will be merged into master?

[nfischer]

@detailyang thanks for the follow up. I'll try to put in some work on this PR this week. I believe most of the blocking issues are now resolved.

This just needs to be rebased on top of master and the tests need to be rewritten to use ava.

[nfischer]

This is now rebased and tests are refactored.

This is currently blocked on #633, and I need to check that I've properly taken advantage of the PRs that this was previously blocked by.

[nfischer]

TODO list (in no particular order):

• possibly rename realtimeOutput to interactive (taken from this comment)
• fix TODOs in the code
• get rid of nodeBinPath (this was taken care of in Move execPath into common #633)
• make sure it handles interactive input nicely
• clean up commented code
• deprecate exec
• rebase off dev
• merge into dev
• fix skipped tests
• write tests for interactive output (maybe check that ls --color=auto prints colored output)
• decide whether or not to return control characters in the return properties (like colors inside result.stdout)

[zhiqunq]

Hi, @nfischer.

what's time your PR will be merged into master?

[nfischer]

@zhiqunq this is still slated for v0.8. I haven't had time to pick it back up. I'm waiting to wrap this up so that I can be confident it fixes all of the major issues with exec. If you'd like to help, feel free to test this out and see if it meets your needs or if it has any issues.

[wclr]

Hope to see this soon!

[nfischer]

Unfortunately, I don't have a whole lot of time for OSS right now, so this will probably take a month or two.

[bruce-one]

Should this use process.exitCode = code instead?

Based on the warning from https://nodejs.org/api/process.html#process_process_exit_code

It is important to note that calling process.exit() will force the process to exit as quickly as possible even if there are still asynchronous operations pending that have not yet completed fully, including I/O operations to process.stdout and process.stderr.

(Which seems to imply it might not actually let the streams close/flush correctly?)

[nfischer]

This is better, thanks!

On second thought, I'm not completely sure if delayedExit would've worked. It posts a timeout to work around this, but that might not actually work if streams are buffered after the process finishes.

[shaunbaruth]

@nfischer Thanks for your work on this. It's a very useful feature. Looking forward to it.

[brennoo]

@nfischer thanks for good work here, we still good to have it in v0.8 ?

[nfischer]

Update:

I'm trying to harden shell.exec() a bit for the v0.8 release (#782). It'll have some (not all) of the improvements from this PR (source code script instead of writing to a file, reducing file IO), but the goal is a backwards-compatible API.

I almost have that working, and it's cleaning up quite a bit of code. After we land that, there should be a little bit of clean-up work, and then it should be easy to revisit this PR (and build a secure-by-default API!).

Thanks for your patience!

[nfischer]

Bumping this back to v0.9. I want to release the hardened exec changes under v0.8 (still a minor security improvement). This will give us a chance to fix any bugs, and move forward with the new API.

[evenstensberg]

Any update for this?

[nfischer]

Updates:

• I think Harden shell.exec by writing the child process in a source file #782 is wrapped up (I need to double check).
• We want to do our v0.8 release without this new API, but including the improvements from Harden shell.exec by writing the child process in a source file #782. This will give us a chance to battle test the changes and make sure there are no issues.
• After that, this PR can become a lot smaller and more manageable--and we can focus on merging it and pushing a release

[nfischer]

Ok, so v0.8 is released! If you can, please try out the release, let us know if there are any new issues with exec. I'm going to dedicate time over the next couple weeks on this PR.

[igrayson]

We're looking forward to switching over to this more secure execution method! Any update?

[nfischer]

@igrayson I'm currently finishing up a local prototype. I will post a design doc in the next 2-ish weeks and upload a PR around the same time. Sorry this has dragged on so long, but this is indeed still on my radar (and thanks for asking about progress).

[nfischer]

Thank you all for your patience. I'm currently polishing up my prototype (it is going to take a slightly different approach from this PR). I've written a design doc, and I invite you all to comment on the design (in particular, please check the proposed design will work for your use case).

Next steps:

• upload my prototype in a new PR, close this one
• land the initial prototype, and implement the extra "modes" mentioned in the design doc in further PRs
• the first shelljs release with shell.cmd() will be the first release we deprecate shell.exec()

[nfischer]

New PR is up at #866! Closing this PR (and the other PR should be in a much more landable state than this one).

[nfischer]

If you've found this PR, please do not consider this a security fix. ShellJS does not have a security "vulnerability" to resolve, we have a poorly-designed API (shell.exec()) that must be used with care to be considered secure. We've updated documentation to warn against improper usage and have provided security guidelines to help dependent modules use this safely. Please consult #945 for more information.

---

This PR is an obsolete attempt to provide a better API, but such a better API is not required for your modules to be secure.