Unable to overwrite defaultWebPreferences

[JulianGlueck]

What were you trying to do?

I am trying to overwrite certain Electron webPreferences for a new window.
In my case, I want to disable the nodeIntegration flag. By default, NativePHP will enable this flag. This is considered unsafe when navigating to external sites (see this page).

Fortunately, NativePHP allows to pass custom web preferences. The problem is though, that it defines defaultWebPreferences and spreads it with the custom ones. Yet, the default ones are spread after the custom ones which means that you cannot overwrite the defaults. See line 301 in the window.ts file.

In my opinion, the current implementation is not correct. Lines 302 and 303 in windows.ts should be switched. But I am unsure, if there was any consideration behind that placement.

I will be happy to create a PR for this issue but wanted to check back with you guys first. 🙂

What happened?

Custom webPreferences will be ignored if already defined in the defaultWebPreferences.

How to reproduce the bug

Simply open a new window and pass any webPreferences that are defined in the defaultWebPreferences like so:

Window::open()
    ->webPreferences([
        'nodeIntegration' => false
    ])
    ->url(...)

These will not be considered.

Debug Output

n/a

Which operating systems have you seen this occur on?

No response

Notes

No response

[gwleuverink]

I'm afraid I don't have the history of this decision. If I'm not mistaken we rely on node integration for some things in the preload script. I'm not 100% sure though - It's worth disabling if we don't need it, but this will be a breaking change for projects relying on this.

Other than that after inspecting te default preferences I don't see anything that wouldn't be okay to overwrite. With the exception of the preload script path. So it think it'll be okay to flip those arguments as you suggested.

[JulianGlueck]

Thanks @gwleuverink (and sorry it took some days..)! I have created a PR here: NativePHP/electron#253

I can confirm that NativePHP won't work properly anymore for custom routes when you disable certain default web preferences (e.g. you cannot listen to Events anymore within a window via Livewire or JS). So overwriting settings like nodeIntegration, sandbox and contextIsolation should only be done when navigating to external sites.

I am thinking of creating a separate PR that would allow you to switch to a "safe mode" for external contents. This would automatically switch the three values mentioned above into a safe default. Guess it would make sense to instruct developers to turn this on when navigating to external routes. What are your thoughts on this?

[simonhamp]

As you've spotted, the reason the default settings are applied to overwrite any custom ones is because they contain various options that are critical to NativePHP's function. Exposing the ability to customise these could lead to settings that break your application, so I took the view to force their state until we had a better way to handle it.

Specifically with nodeIntegration, we clearly rely on loading dependencies in scripts loaded by the renderer (probably mostly as a convenience), so simply turning this off will break some stuff.

I agree that we should aim to disable this by default tho for security purposes 👍🏼 we just need to resolve the breaks before we can release that change.

That said, @JulianGlueck, if your app is working fine with the changes made in your PR then I suspect it's going to be ok and we just need good docs around the implications of changing some of these 'core' settings.

[gwleuverink]

Hey I just tested this out and made it work silky smooth without nodeIntegration using the contextBridge API 🎉

For this to work we'll need to force contextIsolation to be true

The only imports we're using in preload are ipcRenderer & @electron/remote. These work inside a preload script without enabling node integration.

---

The only issue I had was a race condition where the Native helper is not loaded yet. I'm adding a window event we can use to register the listeners inside, this will fix the race conditions 👌🏻

document.addEventListener('native:init' function() {

    Native.on("Native\\Laravel\\Events\\Windows\\WindowBlurred", (payload, event) => {
        //
    });
})

---

I suggest we change the default config to have contextIsolation: true and nodeIntegration: false.

Then make everything overwritable by the user (like PR'ed by @JulianGlueck ) except the preload path & contextIsolation. We need those two to stay as they are.

This will be a breaking change however. Some folks might rely on having nodeIntegration enabled by default. I suggest we add this to the upcoming v2 release @simonhamp if you're okay with what I mentioned above

PS: I also see that we're attaching remote to the window object. We're not relying on this anywhere I think. I'll remove that too since we're introducing breaking changes anyway. If we're exposing stuff to the window it should be very specific.

[gwleuverink]

Okay so I've changed some things around so our preload script works with these enhanced security preferences

NativePHP/electron#255

I still need to change the order of the defaults so some of these can be overwritten. Using this approach we can't touch the preload, contextIsolation & sandbox preferences. The rest can be changed 👌🏻

What do you guys think?

[JulianGlueck]

Awesome @gwleuverink, this looks great! 🙂

[gwleuverink]

Added it to the docs as well: NativePHP/nativephp.com#209

[joshua-jojo]

This topic is interesting, I hope that contextIsolation can be implemented, I am an electron js user for the application I developed with the use of serial reading features, bluetooth and various hardware, if this is implemented then I don't need to work harder.

[gwleuverink]

Context isolation is enforced in the linked PR 👍

[gwleuverink]

@JulianGlueck you got a shoutout in the latest dev update

Didn't know where else to contact you 😅

[JulianGlueck]

@gwleuverink Nice, thank you! =)