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

[Discussion] Changing the defaults for nodeIntegration and contextIsolation to improve the default security posture of Electron applications #23506

Closed
MarshallOfSound opened this issue May 11, 2020 · 62 comments

Comments

@MarshallOfSound
Copy link
Member

Planned Changes

Change the default of contextIsolation from false to true

Without contextIsolation any code running in a renderer process can quite easily reach into Electron internals or your preload script and perform privileged actions that you don't want arbitrary websites to be doing.

For more information on contextIsolation, how to enable it easily and it's security benefits please see our dedicated Context Isolation Document.

We're making this change to improve the default security of Electron apps so that your app is only insecure if you have deliberately opted in to the insecure behaviour.

Timeline

  • Deprecate the current default of contextIsolation in Electron 10
  • Change to the new default (true) in Electron 12

Remove the nodeIntegration flag completely

Historically we have recommended that apps use nodeIntegration: false to prevent renderers from having access to Electron internals or the require function. Over time it has become clear that this flag actually has negligible security impact and can easily be bypassed. This was the original motivation for adding the contextIsolation flag.

We are now confident enough in the contextIsolation feature that we intend to remove the misleading nodeIntegration flag and instead strongly recommend usage of contextIsolation.

Timeline

  • Deprecate the flag and instruct folks to use contextIsolation: true instead of nodeIntegration: false in Electron 10
  • Remove the flag and any effect it had in Electron 12
@reZach
Copy link

reZach commented Jun 4, 2020

This is marked as discussion, so I hope I'm allowed to post, if not, please limit to just collaborators.

I feel this is a good change, but not many are prepared for. There are a lot of packages/templates that make use of remote and do not use IPC to talk between the processes. I've built a template that's supportive of this change, but I feel a lot will be discussing once this change is implemented.

@PalmerAL
Copy link
Contributor

I'm not sure if this has been discussed anywhere (or if I'm not thinking through it correctly), but it does seem like there's a scenario in which:

  • You're creating a webContents with nodeIntegration: false, contextIsolation: false in an old Electron version.
  • You upgrade directly from an old version to 12, or you don't read the deprecation warnings in the intermediate versions.

The effect would be that node integration would be silently enabled in your app after the upgrade, which would potentially cause severe security issues. (I suppose that wouldn't matter if nodeIntegration can already be bypassed, but as far as I know it's still reasonably secure as long as you don't have a preload script).

As a solution for this, would it be possible to check if nodeIntegration: false is set on a webContents, and throw an error (and not create the contents) if it is?

@MarshallOfSound
Copy link
Member Author

That's a reasonable point, I don't think many people would explicitly say contextIsolation: false but we should probably handle that in some way @nornagon

@nornagon
Copy link
Member

nornagon commented Jun 24, 2020

nodeIntegration: false is not very much protection at all. I spent an hour this morning looking and found at least one attack vector that provides access to internal methods that nodeIntegration: false doesn't protect against. Ultimately, there is no reliable way to make having node and web content in the same context safe. If you care at all about security, contextIsolation: true is the way to go (and, ideally, sandbox: true as well).

I can imagine throwing on nodeIntegration: false being a better way of alerting to the developer that they need to change something, though!

@pushkin-
Copy link

pushkin- commented Jul 3, 2020

@MarshallOfSound From some previous discussion with you, you mentioned that contextIsolation should be enabled for all BrowserWindows/BrowserViews regardless of whether it has a preload script or not. You told me that it should be enabled even when loading only local content.

The docs currently make it seems like this is only useful when there's a preload script, or you're loading a URL. Should they be updated to indicate that it should be on everywhere (even though that will be the default behavior anyway)

@ivanjeremic
Copy link

ivanjeremic commented Jul 7, 2020

Does this mean Native modules will not work anymore in electron? I just added contextIsolation: false, to make it work again but got same error. Loading non-context-aware native module in renderer:

@nornagon
Copy link
Member

nornagon commented Jul 7, 2020

Does this mean Native modules will not work anymore in electron?

No.

Loading non-context-aware native module

See #18397

@ivanjeremic
Copy link

Does this mean Native modules will not work anymore in electron?

No.

Loading non-context-aware native module

See #18397

This doesn't help.

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Oct 13, 2020

Sounds like a great change to me. I'm always for security-by-default and always wondered why nodeIntegration exists to begin with.

Anyway, what about nodeIntegrationInSubFrames? I feel like this would've been a great moment to normalize it as well. There's a lot of discussion about it in #22582. It is ill named and has the side effect of enabling preload in subframes. It looks like this was one of the intentions but the name doesn't reflect that. When nodeIntegration is removed it only makes sense to remove it's sibling nodeIntegrationInSubFrames as well. And instead rename it to preloadInSubFrame (as a boolean) or even framePreload (pointing to a script to be used as preload in frames). And also make sure it doesn't actually enable node integration but only preload.

Same with nodeIntegrationInWorker btw. It'd be weird to keep them around (at least with the same name) when nodeIntegration is removed.

hmedney pushed a commit to hmedney/electron-react-starter that referenced this issue Nov 24, 2020
pascalbayer added a commit to pascalbayer/nx-electron that referenced this issue Nov 26, 2020
@RAMKUMARDANE
Copy link

Planned Changes

Change the default of contextIsolation from false to true

Without contextIsolation any code running in a renderer process can quite easily reach into Electron internals or your preload script and perform privileged actions that you don't want arbitrary websites to be doing.

For more information on contextIsolation, how to enable it easily and it's security benefits please see our dedicated Context Isolation Document.

We're making this change to improve the default security of Electron apps so that your app is only insecure if you have deliberately opted in to the insecure behaviour.

Timeline

  • Deprecate the current default of contextIsolation in Electron 10
  • Change to the new default (true) in Electron 12

Remove the nodeIntegration flag completely

Historically we have recommended that apps use nodeIntegration: false to prevent renderers from having access to Electron internals or the require function. Over time it has become clear that this flag actually has negligible security impact and can easily be bypassed. This was the original motivation for adding the contextIsolation flag.

We are now confident enough in the contextIsolation feature that we intend to remove the misleading nodeIntegration flag and instead strongly recommend usage of contextIsolation.

Timeline

  • Deprecate the flag and instruct folks to use contextIsolation: true instead of nodeIntegration: false in Electron 10
  • Remove the flag and any effect it had in Electron 12

As I am providing a few small apps that works with nodeintegration set to true for realm db, react js, babel and webpack,
question is how I can then "set nodeintegration to true" ?

@Nantris
Copy link
Contributor

Nantris commented Jul 21, 2022

I think that the docs on security could use a little bit of updating, or maybe it's my knowledge that needs updating.

@MarshallOfSound thanks so much for all your explanations in this thread; I'd be very grateful if you could correct any misunderstandings I have here or confirm that the docs could use some changes.

https://www.electronjs.org/docs/latest/tutorial/security

  1. It suggests contextIsolation is only needed when loading remote content, but I think it should really be whenever loading remote content or user generated content at least, if not always?

  2. Even though contextIsolation is only recommended for remote content, enabling sandbox is suggested as something that should be done for all apps ("You should at least follow these steps to improve the security of your application"). I would think that if sandbox is being recommended as a minimum for all apps then contextIsolation certainly should be too, no?

From the Sandbox tutorial:

"Note that because the environment presented to the preload script is substantially more privileged than that of a sandboxed renderer, it is still possible to leak privileged APIs to untrusted code running in the renderer process unless contextIsolation is enabled."

  1. I assume that should read "unless contextIsolation is also enabled; because as written it seems to suggest that contextIsolation alone would be sufficient to prevent privileged APIs being accessible to untrusted code, even without sandbox

On a related note, if everything Node related should be handled in the main process and blocking the main process causes all renderer windows to freeze, am I wrong to think special considerations might be needed from developers when running node code in the main process to avoid blocking it, and can you make any recommendations for handling this?

@pushkin-
Copy link

@slapbox The "for remote content" bit was removed in this commit actually, but maybe there's some other document that needs to be updated? Or it hasn't been "released" yet?

@oalfroukh
Copy link

Hi,
I'm checking the side effects when enabling the contextIsolation on my project, to be closer to Electron security recommendation there are multi-methods that are declared on the renderer side (I'm using loadURL to generate Eelctron desktop app which depends on the website).
So I'm calling these methods from the preload scripts, so all the logic for our app and services on the renderer side (web app), currently we are using these services from the preload script, but when enabling the contextIsolation, I can not use the renderer services (I know I can create a bridge, but it's one-directional, the renderer side can use the APIs via that bridge which declared from preload script, while the preload script can not use any method from renderer).

Anyone has an idea to avoid these issues because there are hundreds of services on our app which are declared on our website side and Irreplaceable for the desktop app side?

@Nantris
Copy link
Contributor

Nantris commented Sep 22, 2022

while the preload script can not use any method from renderer

It can, in an indirect way - but it's quite tedious - too much so for me to outline here - but basically you need to pass any data you need from the renderer into your preload functions as arguments. If you have hundreds of services to tend to: pain.

Adam777Z referenced this issue in Adam777Z/nativefier Nov 9, 2022
… to contextIsolation:true (PR nativefier#1308)

Copy-pastaing details from [Electron 12 breaking changes](https://www.electronjs.org/docs/latest/breaking-changes#planned-breaking-api-changes-120):

> ### Default Changed: `contextIsolation` defaults to `true`[​](https://www.electronjs.org/docs/latest/breaking-changes#default-changed-contextisolation-defaults-to-true "Direct link to heading")
> 
> In Electron 12, `contextIsolation` will be enabled by default. To restore the previous behavior, `contextIsolation: false` must be specified in WebPreferences.
> 
> We [recommend having contextIsolation enabled](https://www.electronjs.org/docs/latest/tutorial/security#3-enable-context-isolation-for-remote-content) for the security of your application.
> 
> Another implication is that `require()` cannot be used in the renderer process unless `nodeIntegration` is `true` and `contextIsolation` is `false`.
> 
> For more details see: [https://github.com/electron/electron/issues/23506](https://github.com/electron/electron/issues/23506)

I find the security drop acceptable, as reverting the new Electron 12 isolation brings us to the previous level of security, and I don't have the time/will to keep the isolation and migrate to the newer better safer thing that Electron >= 12 wants.

Co-authored-by: Radomír Polách <rp@t4d.cz>
rajr5 pushed a commit to rajr5/nx-electron that referenced this issue Jun 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests