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

[v2] Prefer using preload script for renderer dependencies - no longer require nodeIntegration #289

Open
matmalkowski opened this issue Jan 13, 2021 · 11 comments · Fixed by #300
Labels
enhancement ✨ security Pull requests that address a security vulnerability v2 All issues related to v2
Milestone

Comments

@matmalkowski
Copy link
Collaborator

As the title suggest, move dependencies to preload script, similar to how it's done in the fork

@matmalkowski matmalkowski added enhancement ✨ security Pull requests that address a security vulnerability v2 All issues related to v2 labels Jan 13, 2021
@matmalkowski matmalkowski added this to the 2.0 milestone Jan 13, 2021
@Nantris
Copy link
Contributor

Nantris commented Feb 8, 2021

@matmalkowski I've been looking into best practices and experimenting for the past week or so, and I think the best solution is to do what's done in Secure Electron Template and allow users to pass in the necessary dependencies on their own from their own preload script like this https://github.com/reZach/secure-electron-template/blob/master/app/electron/preload.js#L15

And then in the contextMenu package the code looks like this https://github.com/reZach/secure-electron-context-menu/blob/master/src/index.js#L30-L103

This gets us the benefits of the preload script method without locking users into using preload script provided by electron-redux. As I understand it Electron can only support one preload script, so this is a really important aspect.

One thing I'm still pondering though is how to "supporting" contextBridge, as opposed to "requiring" contextBridge. Can you see a way to architect the code so that users will be able to choose whether to use contextBridge for themselves?

I hope that can be achieved, but if not, I would propose a v3 of this library once v2 is ready; basically the same code but with contextBridge required and users having to pass their own bindings in their preload script.

PS: The improvements from v1.x to v2 are astounding!

@Nantris
Copy link
Contributor

Nantris commented Feb 8, 2021

I'd like to work on this but I'm a Typescript noob so I'm not sure how useful I'll be.

It seems to me that the only dependency we need to bind is ipcRenderer.

The files that use ipcRenderer are:

@matmalkowski what would you think of a JS class file like bindings.js to handle registering the binding of ipcRenderer, and then we import bindings from that file and we use it like bindings.ipcRenderer('on', ...args) or bindings.ipcRenderer('sendSync', ...args) - something like:

Bindings.js

class Bindings {
  register = ipcRenderer => {
    this.ipcRenderer = ipcRenderer;
  };

  ipcRenderer = (funcName, ...args) => this.ipcRenderer[funcName](...args);
}

export const bindings = new Bindings();

@Nantris
Copy link
Contributor

Nantris commented Feb 8, 2021

Oh and maybe #235 should be closed in favor of this issue.

@matmalkowski
Copy link
Collaborator Author

@slapbox I'm not sure (I got to do some reading myself on that topic) but seems to me like we could just declare our scoped preload script as a module, and expose it as part of the lib, users can still consume it in 2 ways, considering the limitation you mentioned of single preload script:

  • no custom / user defined preload -> use the module as the preload script
  • custom preload script => import the module at the top of your script

That would work as well right?

@Nantris
Copy link
Contributor

Nantris commented Feb 17, 2021

I myself am not sure but I think you're right. How to architect the library in a DRY way while supporting that eludes me though.

I also was wondering if we could do something like:

    register = ipcRenderer = require('electron').ipcRenderer => {
    this.ipcRenderer = ipcRenderer;
  };

But, given the fact that Electron itself is moving to eliminate the possibility of using this library without using a preload script (because that would require nodeIntegration which is going away) maybe the need to accommodate both simultaneously isn't even necessary.

Maybe v2 could be provided for users still using nodeIntegration and v3 could rely entirely on it being loaded via a preload script. It seems like the headaches of supporting both may not be worth it for the maintainers, but that's your call.

Thoughts?

@matmalkowski
Copy link
Collaborator Author

@Superjo149 I have started to work at this issue, and it seems your change for unified interface for initialisation (end re-use of some of the methods as shared will need to be removed for the favour of security.

Problem is that we have to have clear separation between renderer code & main code on build time when it comes to referencing electron (or any other dependencies for that matter). Right now with your changes it will bundle main process related modules into the renderer module because fork is done on runtime.

@matmalkowski
Copy link
Collaborator Author

@slapbox if I manage to finally write this (and I'm in progress) I don't plan to support the old way with nodeIntegration: true in mind. Configuration-wise the new API should still be simpler than v1, it's not removing any functionality related to it, so I don't see reason why we should support insecure interface

@sneljo1
Copy link
Contributor

sneljo1 commented Feb 23, 2021

@Superjo149 I have started to work at this issue, and it seems your change for unified interface for initialisation (end re-use of some of the methods as shared will need to be removed for the favour of security.

Problem is that we have to have clear separation between renderer code & main code on build time when it comes to referencing electron (or any other dependencies for that matter). Right now with your changes it will bundle main process related modules into the renderer module because fork is done on runtime.

Ah, yes I understand. Let's do what's best right 👍

@Nantris
Copy link
Contributor

Nantris commented Feb 23, 2021

Configuration-wise the new API should still be simpler than v1, it's not removing any functionality related to it, so I don't see reason why we should support insecure interface

@matmalkowski that makes sense!

@matmalkowski
Copy link
Collaborator Author

@musou1500, @slapbox I managed to get it working, most troublesome part was spectron & e2e tests...

I created a draft PR (#300 ☝🏻 ), will still need to update some docs for this & add JSDOC comments to new API, but let me know what do you think, since the feature request came from you 😄

@Nantris
Copy link
Contributor

Nantris commented Mar 3, 2021

Looks promising!

Because I (apparently) can't compile TypeScript on Windows it's tough for me to give it a quick test, but I looked over the PR just now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ security Pull requests that address a security vulnerability v2 All issues related to v2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants