Experimenting with the plugin system, I had a few thoughts that I'd like to discuss with the developers:
Is node integration needed for plugins?
Currently, both the main plugin instance and the content scripts have access to the
require function and all of nodejs's built-in modules, including
fs. I think this is an unnecessary risk, as non-malicious plugins should have no need for this feature.
For the window, in which the main plugin code is running, getting rid of it seems straightforward: we could simply turn off the
nodeIntegration flag, and move the sandbox proxy code, which depends on require, to a preload script. I think this could be done while preserving the plugin API. What do you guys think?
Content scripts are more tricky, on the other hand. I suppose the most bulletproof way would be to only run them in iframes. For CodeMirror scripts, I think this would mean wrapping the entire editor into a sandbox, but for markdown plugins, I'm not really sure. Maybe they could run in the iframe where the rendered markup is displayed? Either way, this would be a much bigger task than the main plugin.
Another idea I had, was to put the content scripts into a wrapper which overwrites all possible ways to reference require with undefined (i.e.
require, window.require, self.require etc). This has a ton of edge cases and potential ways to get around it, so it will never be as secure as an iframe, but it would require minimal changes.
Plugins crashing Joplin
Prompted by this bug report, I have done some experiments, and it seems that almost any exception coming from CodeMirror content scripts will crash Joplin. Exceptions thrown at the very top of the module are caught, but CM plugins usually rely on registering callbacks, and errors coming from those are especially tricky to catch. Some are caught by CodeMirror, but most of them not. Here's a gist with a minimal content script that can crash joplin every time.
Of course, moving the markdown editor to an iframe would solve this issue as well. But, an easier solution would be to add an error boundary around the editor. The problem with this is that error boundaries can't catch exceptions coming from
setTimeout and the like, but CodeMirror does call plugin-defined functions from timeouts in a number of cases (e.g. this one came from a timeout too).
One other idea I had was that maybe the CodeMirror object passed to the plugins could be wrapped in a Proxy, which wraps any callback passed to CM in a try-catch. This too has a bunch of edge cases to deal with, for example sometimes callbacks return callbacks or even objects of callbacks.