Electron v11 support

[pierreraii]

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

[jviotti]

Hi @pierreraii ,

Sorry for the delay and hope you had a happy new year. Do you mind posting what the error message is?

[pierreraii]

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

[Nantris]

@jviotti are there plans to support contextBridge for security?

[jviotti]

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:

• Require electron-json-storage directly on the renderer process, or
• Wrap electron-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?

[Nantris]

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

[jviotti]

@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 #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).

[jviotti]

@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?

[jviotti]

@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
})

[jviotti]

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'))

[jviotti]

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.

[jviotti]

I published v4.4.0 with the latest fix

[Nantris]

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.

[jviotti]

@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!