TritonDataCenter / node-workflow

Task orchestration, creation and running using NodeJS
MIT License
455 stars 105 forks source link

allowing use of require() in tasks #154

Open TomKaltz opened 8 years ago

TomKaltz commented 8 years ago

I would like to use require() in my task body. I have found a way to hack this capability by adding... require: require

to https://github.com/joyent/node-workflow/blob/58715e9835d80156370035354143acb2add92c86/lib/task-runner.js#L53

This allows me to use require() relative to the task-runner.js file of the wf module. Including a script from my project root would be require('../../../thescript')

I had to modify the source of this module to get require to work properly because specifying require: require in the runner sandbox modules config doesn't work.

I understand that the use of vm is for security purposes but is there a safe way to allow the use of require in task body? Perhaps allow the user to specify require in runner config with a disclaimer that doing so would lower safety?

kusor commented 8 years ago

Honestly, I don't think it's a good idea to allow the use of require from tasks: what is the reason to do not require those modules in advance using wf-runner configuration?

Not to mention that we would be moving from "dangerous" to "completely unsafe" in terms of executing completely arbitrary code from vm.

TomKaltz commented 8 years ago

I would like to be able to reuse code and reduce boilerplate in my tasks. As far as security, I don't understand how this is any less safe. Your database and file system already hold 'arbitrary' code that is run anyway. We just don't have the option to require arbitrary files for our arbitrary code. Not sure how sandboxing in this case makes anything any safer. I don't understand where malicious injection would occur otherwise.

kusor commented 8 years ago

@TomKaltz allowing a task to use require means that it could also require things like fs and, for example, fork a child process, which could mess pretty much everything regarding how wf-runner work.

Anyway, I wouldn't be opposed to add a configurable option to use require and have it disabled by default with that disclaimer you mentioned