-
Notifications
You must be signed in to change notification settings - Fork 48
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
fix!: change enableRemoteModule to .enable() for electron@14 #72
Conversation
…enableRemoteModule check for electron >= 14
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please undo the unrelated changes to README.md 🙇♂️
I think this is a reasonable approach, but I wonder if there is some way to allow users of this module to enable the remote module for only some WebContents. Perhaps some new api like
require("@electron/remote/main").permit(webContents)
?
Sorry, it seems like auto-formatting kicked in - now the unrelated parts of README are back in the original state |
That is true, indeed it would be sensible to restrict remote to selected WebContents. As there seems to be no way to operate on the WebPreferences anymore, perhaps even re-using the original |
@artus9033 yeah, that makes sense! Do you want to take a stab at it? |
Sure, just testing the implementation now - a commit will fly in in a moment |
Done, this is definitely more solid than the first approach. |
This will break code on Electron >= v14 that still have |
And the I suggest adding a global option like And keep the enableRemoteModule check on all versions for backward compatibility. |
I think it doesn't make sense. I tested code on Electron >= v14 that still have enableRemoteModule set true, but @electron/remote raised It seems that enableRemoteModule is omitted from webPreferences on Electron >= v14 even if user set it, so there's no way to let them survive. |
Although the |
It does work for me in browser process at least. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable! Would be great to test.
Ideally, this approach should also work on electron@13
and earlier so people can upgrade @electron/remote
to the new approach while still using an older version of electron
, to allow for future-proofing.
Sure, this is prepared to be fully working with older releases. I tested it locally on older versions and it fully maintains old API compatibility. In the next few days I can try to port the tests to the newest versions. @nornagon, should I put them here in this PR or is this one finished for now and the tests would be a separate PR? |
@artus9033 @nornagon It's not electron making breaking changes, but @electron/remote that is about to make one. At least make that a major release, please. |
Co-authored-by: Jeremy Rose <nornagon@nornagon.net>
I think this is a reasonable point. Unfortunately we have to compromise somehow here. In Electron 14.x, WebPreferences no longer record unknown parameters, which is what was allowing
The former approach is a more secure approach, as it means that access to The latter is a more permissive approach, allowing apps to continue to function with minimal changes, but potentially introducing security issues (i.e. granting I'm not sure how common it is in apps which use I'm also hesitant to publish an upgrade, especially a minor-version bump, which would further reduce the security of apps using this module. On the one hand, Finally, given that with Electron 14, currently For the above reasons, I think the best way forward is to do the following:
I think this strikes the best balance between security and convenience. We can gently guide users towards the appropriate fixes, and hopefully make it as smooth as possible for them to change to the new API, and also avoid accidentally creating new security holes. |
I've added a draft migration guide in #74. In that guide I changed the name of the function from |
@nornagon that sounds perfectly reasonable. As an example, in an application I created (which is publicly available on one of my repos), I use separate I've just renamed the |
Hi -- Why this merge is blocked? I really need the update for electron 14 (since it updates things that I am using). Please let me know if you want me to do that, or just adjust the relevant things? Thanks. |
BREAKING CHANGE: This removes the `enableRemoteModule` option in favor of a new `.enable()` method for enabling the remote module on a particular WebContents. See https://github.com/electron/remote/blob/main/docs/migration-2.md for migration instructions. This is an empty commit for forcing a release after a semantic-release failure in fd928e5.
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
With Electron 14.0.1, Am I missing something, @nornagon? I wouldn't a breaking change in a point release, and this code worked just fine with v14.0.0. Thanks for any help. Updated, figured it out, it has to be |
BREAKING CHANGE: This removes the
enableRemoteModule
option in favor of a new.enable()
method for enabling the remote module on a particular WebContents. See https://github.com/electron/remote/blob/main/docs/migration-2.md for migration instructions.Fixes #71