feat: add preloadInWorker API

[hamst]

Description of Change

Add preloadInWorker for webPreferences in BrowserWindow to define a script to be loaded first before any other scripts in worker.

cc @codebytere

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation is changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Added preloadInWorker to define a script to be loaded first before any other scripts in worker.

[welcome]

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[zcbenz]

From the document it is unclear how this option works with contextIsolation, can you make it clear and add related tests?

And I think this option should either respect the contextIsolation option of the window, or just always run the preload script in isolated context. /cc @electron/wg-api

[zcbenz]

This file shares lots of code with lib/renderer/init.ts, can you move the shared code into a utility function? Like what windowSetup and webViewInit do.

[hamst]

@zcbenz It seems that contextIsolation is not supported in WebWorkers, so adding preloadInWorker doesn't affect it in any ways. I will add some details in documents, as far as having tests with contextIsolation, will they be useful?

[zcbenz]

The contextIsolation controls whether preload scripts should run in the web page's context or in a separate context, currently it is not related to WebWorker because WebWorker does not have preload scripts.

But since this PR is adding preloadInWorker, we have to define how contextIsolation affects preload scripts in WebWorker. We can either:

1. Make preloadInWorker respect the contextIsolation option that, contextIsolation should be able to control whether preload scripts should run in the worker script's context or in a separate context ;
2. Or ignore the contextIsolation option and always run the preload scripts in separate context in WebWorker.

From current implementation, the preload scripts specified by preloadInWorker are running in the same context of the worker script, which is against the "secure by default" principle that we have in Electron's APIs.

[MarshallOfSound]

Without solving the ctx isolation issue:

This script will always have access to node APIs no matter whether node integration in worker is turned on or off.

is fundamentally a massive security vulnerability that we can't / won't land into this project. Putting a hard block on this pending a proposal for how to deal with ctx isolation

[hamst]

Right now we have an option nodeIntegrationInWorker but don't support contextIsolation in worker. Being enabled nodeIntegrationInWorker allows access to powerful API without any limitation for all web worker scripts loaded in webpage, and there is no way to limit this. Very unlikely that this option without contextIsolation support follows "secure by default" principle, and that's what we have today. So contextIsolation for worker must be supported no matter if we have preloadInWorker or not.
Option preloadInWorker makes things way better: you define the script with all access to API, and you can make some API visible for all workers, but it requires additional steps and unlikely could be done unintentionally. By default, nothing will be exposed. That's a big difference from security prospective.
So getting contextIsolation support for workers should be a separate task which must be done to be "secure by defaults" regardless preloadInWorker. As contextIsolation heavily relies on contextBridge module, some work needs to be done to relax the requirement to avoid using electron modules in workers https://www.electronjs.org/docs/tutorial/multithreading#available-apis and make the implementation thread-safe.

[zcbenz]

@hamst Thanks for explaining, I see your point now. I'm good with this API, since it is an improvement over nodeIntegrationInWorker on security, before contextIsolation is supported in worker.

[hamst]

Would be good to link the issue #28620 to this PR.

[MarshallOfSound]

since it is an improvement over nodeIntegrationInWorker on security, before contextIsolation is supported in worker.

I disagree with this from a security perspective. Previously, enabling nodeIntegrationInWorker very clearly gave all workers Node.js access. Now providing preloadInWorker transparently provides node.js access to the worker context. This is massively unsafe, isn't clear from docs and people won't realize that's what it does.

Without context isolation this concept of "run a preload next to a worker script" is massively unsafe and IMO isn't something that belongs in core as it is antithetical to our secure-by-default policy