Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Electron v11 support #155

Open
pierreraii opened this issue Dec 28, 2020 · 13 comments
Open

Electron v11 support #155

pierreraii opened this issue Dec 28, 2020 · 13 comments

Comments

@pierreraii
Copy link

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

Screen Shot 2020-12-28 at 12 54 15 PM

@jviotti
Copy link
Member

jviotti commented Jan 7, 2021

Hi @pierreraii ,

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

@pierreraii
Copy link
Author

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
Copy link
Contributor

Nantris commented Feb 10, 2021

@jviotti are there plans to support contextBridge for security?

@jviotti
Copy link
Member

jviotti commented Feb 12, 2021

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
Copy link
Contributor

Nantris commented Feb 12, 2021

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 [email protected]

jviotti added a commit that referenced this issue Feb 18, 2021
Right now this module will crash when being ran on a renderer process
that cannot access the `remote` module for determining a default data
path. After this commit, the module will gracefully require users to
call `.setDataPath()` if `remote.app` cannot be accessed.

See: #155
Signed-off-by: Juan Cruz Viotti <[email protected]>
jviotti added a commit that referenced this issue Feb 18, 2021
Right now this module will crash when being ran on a renderer process
that cannot access the `remote` module for determining a default data
path. After this commit, the module will gracefully require users to
call `.setDataPath()` if `remote.app` cannot be accessed.

See: #155
Signed-off-by: Juan Cruz Viotti <[email protected]>
@jviotti
Copy link
Member

jviotti commented Feb 18, 2021

@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
Copy link
Member

jviotti commented Feb 18, 2021

@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 added a commit that referenced this issue Feb 18, 2021
Right now this module will crash when being ran on a renderer process
that cannot access the `remote` module for determining a default data
path. After this commit, the module will gracefully require users to
call `.setDataPath()` if `remote.app` cannot be accessed.

See: #155
Signed-off-by: Juan Cruz Viotti <[email protected]>
@jviotti
Copy link
Member

jviotti commented Feb 18, 2021

@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
Copy link
Member

jviotti commented Feb 18, 2021

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
Copy link
Member

jviotti commented Feb 18, 2021

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
Copy link
Member

jviotti commented Feb 22, 2021

I published v4.4.0 with the latest fix

@Nantris
Copy link
Contributor

Nantris commented Feb 22, 2021

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
Copy link
Member

jviotti commented Feb 26, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants