Skip to content
This repository has been archived by the owner on Sep 29, 2023. It is now read-only.

Fix notification broken since Nativefier 43 / Electron 12 defaulting to contextIsolation:true #1308

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

exander77
Copy link
Contributor

Notification events are broken. Probably by bumping the Electron version and not handling required changes.
window.Notification is not shared with BrowserWindow and preload if contextIsolation is set.

@ronjouch
Copy link
Contributor

ronjouch commented Nov 17, 2021

@exander77 true, good catch, thx for locating the reason, and thx for the PR. Copy-pastaing details from Electron 12 breaking changes:

Default Changed: contextIsolation defaults to true

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 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: electron/electron#23506

I find the security drop acceptable and am happy to merge this, 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. @TheCleric agreed?


@exander77 an aside: gentle with your communication style please, "bumping the Electron version and not handling required changes" is harsh.

We do look at breaking changes, but we're only humans and accidents happen. Nativefier is between a rock and a hard place: I personally no longer even use Nativefier, I maintain it minimally only to not ship something woefully insecure, and Adam's bandwidth is limited these days.

But standing still is not an option, because there's only one way in web land: forwards; not doing major Electron bumps would keep users insecure. And so, we do them. But by doing them, because it's only the two of us with limited usage / motivation and because we don't have an army of testers, we sometimes miss on stuff and regressions happen. On that note, if you have the time, tests for #1288 welcome.

@ronjouch ronjouch changed the title fix: notifications Fix notification broken since Nativefier 43 / Electron 12 defaulting to contextIsolation:true Nov 17, 2021
@ronjouch ronjouch merged commit ca7d25f into nativefier:master Nov 22, 2021
Adam777Z pushed a commit to Adam777Z/nativefier that referenced this pull request 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: [electron/electron#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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants