davideicardi / live-plugin-manager

Plugin manager and installer for Node.JS
MIT License
242 stars 44 forks source link

Make VM sandboxing optional #39

Closed strogonoff closed 4 years ago

strogonoff commented 4 years ago

I know this may be controversial, but considering that VM may not be foolproof, perhaps an option to not use VM may be useful? E.g., if the host app only allows running vetted plugins.

VM causes issues with plugins intended to run in window context. I have troubles accessing setTimeout, Blob and other native window features. Also it breaks React’s useEffect, although that can be worked around by passing the plugin useEffect implementation from the host (so far I can’t find workarounds for native window features).

(Context: I’m working on a project that uses plugins in Electron, and plugins provide both components that run in Node context and components that run in browser window context.)

strogonoff commented 4 years ago

Basically I’d like to use all the usual APIs like plugin installation capabilities, being able to import host modules from the plugin, etc., the only difference being that plugin code might not run in a VM but in host context (at least when it runs in window). I’d be willing to contribute support for that option, but would appreciate pointers as to where to start.

davideicardi commented 4 years ago

I think that VM is required, because it is the only way (as I know) to run javascript code on the fly. and that allows us to load and unloads plugins at runtime. The alternative is to just use standard require but you will not able to unload plugins easily.

But a lot of users use live-plugin-manager with Electron, so for sure it is an interesting use case. The problem is that I don't have experience on this, so for me it is difficult to help.

What I really appreciate is if you create a simple/minimal example with Electron that clearly shows current problems. Having this as a unit test should be ideal, but if not possible also a simple example like https://github.com/davideicardi/live-plugin-manager/tree/master/samples can be good. In this way I can better debug and investigate these problems.

strogonoff commented 4 years ago

Good point! Will do.

On 9 Oct 2020, at 9:11 PM, Davide Icardi notifications@github.com wrote:

 I think that VM is required, because it is the only way (as I know) to run javascript code on the fly. and that allows us to load and unloads plugins at runtime. The alternative is to just use standard require but you will not able to unload plugins easily.

But a lot of users use live-plugin-manager with Electron, so for sure it is an interesting use case. The problem is that I don't have experience on this, so for me it is difficult to help.

What I really appreciate is if you create a simple/minimal example with Electron that clearly shows current problems. Having this as a unit test should be ideal, but if not possible also a simple example like https://github.com/davideicardi/live-plugin-manager/tree/master/samples can be good. In this way I can better debug and investigate these problems.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

strogonoff commented 4 years ago

I think I have narrowed my issue down to “there appears to be no way of making native methods (such as window.setTimeout, element.addEventListener, etc.) callable within VM”. Accessing within VM any function that reports “native code” when printed in browser console crashes browser window instantly. The worst is that the error is so fundamental it escapes any attempt to be caught and handled. DevTools becomes disconnected before anything can be printed.

(As far as the Electron/React issue, it seems to have been unrelated and appears to have been addressed in my project by adjusting Webpack setup. Only the native function call issue remains.)

strogonoff commented 4 years ago

I think by running VM-loaded module in browser context I was stretching what VM was intended to do, and passing native window functions to VM sandbox would not be possible any time soon. Considering plugin code is trusted, plain require() may be the way to go in browser context. Conveniently, PluginManager’s getInfo()?.location can be used to construct the path to require. Thanks for this useful package!

strogonoff commented 4 years ago

I should withdraw this request, as no-VM loading of trusted code is actually straightforward to do per my previous comment. Plugin manager is still handy for installing plugins from NPM and running plugin’s Node (main thread) code within VM sandbox.