Open pierreraii opened 3 years ago
Hi @pierreraii ,
Sorry for the delay and hope you had a happy new year. Do you mind posting what the error message is?
Hello @jviotti,
The error is something like (Will try to edit this with a screenshot later): 'app of undefined' (Debugger points to remote being undefined in electron.remote.app which triggers the error) and this is caused by setting enableRemoteModule to false
@jviotti are there plans to support contextBridge
for security?
Hey @Slapbox , I'm thinking contextBridge
is a wrapper around electron-json-storage
that client applications can opt-in for. I haven't played with contextBridge
much but as far as I understand, the client application can either:
electron-json-storage
directly on the renderer process, orelectron-json-storage
using contextBridge
and access it as i.e. window.storage
:const storage = require('electron-json-storage')
...
electron.contextBridge.exposeInMainWorld('storage', storage)
The reason why I'm hesitant about making this a default shipped as part of the module is that contextBridge
might add a significant performance overhead for some projects, as both arguments and returns need to be deep copied and many applications using this module deal with significantly large JSON documents.
What do you think?
Hey @jviotti thanks for your reply!
I figured basically the same as you about just using exposeInMainWorld
to expose storage
, but it still requires remote
, which should be phased out I think - especially if it's only being used for app.getPath('userData')
. It seems like this could be worked around by having the user manually pass the string or by including a simple secondary import that sets up an ipcMain.handle
for a channel specifically for getting this piece of data from the main process.
I tested this by cloning https://github.com/reZach/secure-electron-template and installing electron-json-storage, and then exposing it how you did:
const storage = require('electron-json-storage');
contextBridge.exposeInMainWorld("api", {
storage
});
And then I realized I needed to enable remote
and disable the template's filtering of remote-require
, remote-get-builtin
, etc.
What sort of sizes of JSON documents do you think need to be planned for on the upper end? As I understand it the structured clone IPC is extremely efficient but overhead is still overhead. That said, security seems like a more paramount concern for more people in more cases.
My two cents, the plan should probably be to make contextBridge
the default within one or two versions of Electron from when they make contextBridge
the default, that's just sort of the expectation I've developed using Electron since v1
and watching packages try to keep up with their rapid pace of development and security hardening.
contextIsolation
will be set to true by default starting in Electron@12.x
@Slapbox
I figured basically the same as you about just using exposeInMainWorld to expose storage, but it still requires remote, which should be phased out I think - especially if it's only being used for app.getPath('userData'). It seems like this could be worked around by having the user manually pass the string or by including a simple secondary import that sets up an ipcMain.handle for a channel specifically for getting this piece of data from the main process.
That is a great point! I've sent https://github.com/electron-userland/electron-json-storage/pull/159 to try to auto-detect if the remote module is available from a renderer process and if not gracefully fallback to requesting the user to manually set a data path. I couldn't test it yet but I suspect that you can just check if remote
and remote.app
are truthy.
Do you happen to know if there is a way to run electron-mocha
in a way such that remote
is not available so we could extend the test suite to run all the test cases in this manner?
What sort of sizes of JSON documents do you think need to be planned for on the upper end? As I understand it the structured clone IPC is extremely efficient but overhead is still overhead. That said, security seems like a more paramount concern for more people in more cases.
The IPC mechanism is probably very efficient. The less efficient part that I worry about is deep cloning JavaScript objects/arrays before sending them through IPC. This means that we would be effectively serializing/deserializing objects/arrays twice (once for IPC, and another one to store/retrieve from files).
My two cents, the plan should probably be to make contextBridge the default within one or two versions of Electron from when they make contextBridge the default, that's just sort of the expectation I've developed using Electron since v1 and watching packages try to keep up with their rapid pace of development and security hardening.
Yeah, I think that makes sense given the direction Electron is taking. I'll try to send a PR making this the default so we see how it performs, and merge it if everything is fine (probably with a major version bump).
@Slapbox As an aside, you are doing a lot of work around this module, so I wonder if you would be open to co-maintaining this project. I believe I can you invited into this org if so. What do you think?
@Slapbox The PR I sent you seems to make the module work fine with contextIsolation
. I tested this out on a BrowserWindow
with:
nodeIntegration: false,
contextIsolation: true,
enableRemoteModule: false,
And whose preload script runs:
const { contextBridge } = require('electron')
const storage = require('electron-json-storage')
contextBridge.exposeInMainWorld('api', {
storage
})
I guess that what we can do is detect if electron.contextBridge
is defined, and if so run:
contextBridge.exposeInMainWorld('storage', require('electron-json-storage'))
However keep in mind that I believe this does need to be done from within the preload script. So we would have to update the README to tell people to just add the following line to their preload script:
require('electron-json-storage')
Or something like that.
I published v4.4.0 with the latest fix
Unfortunately I'm not sure about electron-mocha
but holy cow you already got this fixed before I even got a chance to reply!
I'd be open to helping to maintain, but I have to mention that I've got no experience with open source projects besides one-off pull requests and rambling in the issue trackers. I'd be happy to help how I can though.
I'm looking forward to giving your new PR a try! We have a few other packages we still need to get ready for contextIsolation
before I can turn it on to see just how many things explode.
On the topic of what should be in the README, I think that we should mention best practices are to expose as little of the module as you need. So what we've ended up doing is something like this:
const jsonStorage = {
set: (...args) => electronJsonStorage.set(...args),
getAsync: async (...args) =>
new Promise((resolve, reject) => {
electronJsonStorage.get(...args, (err, data) => {
if (err) reject(err);
resolve(data);
});
}),
};
window.api = {
jsonStorage
}
In particular my concern here was that the storage directory path could be changed. It's a minor risk for sure, but it's perhaps a useful demonstration for people implementing preload/contextIsolation
for the first time, since there's a real lack of accessible resources on how to use these features to their fullest.
Obviously it would be trivial to overwrite a given key with this setup so ideally a user might go even further to hardcode the keys that are allowed to be set
.
@Slapbox
On the topic of what should be in the README, I think that we should mention best practices are to expose as little of the module as you need. So what we've ended up doing is something like this:
Yeah, feel free to send a PR updating the README promoting best practices!
Was upgrading today to Electron v11
Using: electron-json-storage v4.3.0
Even after passing the data path form main_process to the renderer_process, and setting storage.setDataPath() in the renderer_process an error is being triggered by the utils.js line 30 (const app = electron.app || electron.remote.app)
I am calling storage.setDataPath() with the relevant path first thing after requiring the electron-json-storage module