-
Notifications
You must be signed in to change notification settings - Fork 739
Description
Inspired by #524.
Exec has some security concerns, as pointed out in other issues. Due to the API contract, it's inherently vulnerable to shell-command-injection (same as child_process.exec()
). Additionally, due to its architecture, it's also vulnerable to javascript-code-injection. This is because we write out a string (containing JS code) to a file and exec it.
I don't think we have any known security vulnerabilities here, but this is poor design and easy to mess up in future changes.
We should be able to improve the design while maintaining backwards compatibility by adopting the architecture in #524. We can write the child process code in a source file and pass parameters via IPC (more on that later).
This has 3 main benefits:
- Reduces attack surface (no JS injection)
- Child process code is more readable
- Meaningful line coverage for child process code
Care needs to be taken when passing child parameters. It's not valid to pass values on the shell commandline (shells do weird escape regarding \n
, \t
, etc.). I think our best option for IPC is the filesystem. I think IPC modules typically use sockets, but I'm not sure how well these perform for Windows. I think it's acceptable to use the filesystem for now, and come back later with a different IPC system as an enhancement.