[Feature Request]: Enable sandbox: true by default for BrowserWindow

[molant]

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for a feature request that matches the one I want to file, without success.

Problem Description

Since Electron v5 the default behavior of BrowserWindow is to create it with nodeIntegration: false. In v12 contextIsolation was turned on as well (see #23506). Another default that Electron should have is sandbox: true.

Proposed Solution

I'd like the defaults of creating a new BrowserWindow to be equivalent to the following:

const mainWindow = new BrowserWindow({
  webPreferences: {
    nodeIntegration: false,
    contextIsolation: true,
    sandbox: true
  }
);

Alternatives Considered

No alternatives considered. As @MarshallOfSound said in #23506:

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.

Additional Information

Happy to dig and try to find any data needed to make a decission.

[nornagon]

I think we could do this in a fairly gentle and non-breaking way by having the default of sandbox depend on the value of nodeIntegration. i.e. if nodeIntegration: false or nodeIntegration: undefined (i.e. default) then sandbox: true should be the default, but if nodeIntegration: true is specified then sandbox: false should be the default. If we do it that way then I think very few apps will notice the change.

[MarshallOfSound]

@nornagon This is tricky because we have plans to remove the nodeIntegration option --> #23506

[molant]

From what I read, nodeIntegration was supposed to be removed in v12. Is there a new target in mind?

If we are going to break people anyways by removing nodeIntegration maybe we should do everything at the same time?

[nornagon]

iirc plans for removing nodeIntegration are stalled because it's a breaking change without any clear ways to undo the breakage. In particular, AMD modules test for the presence of node globals to determine how to inject themselves onto the page, so this would break a large chunk of the web.

If we do end up deciding to take the plunge and remove nodeIntegration, then we could do that after this change, and have sandbox: false imply the equivalent of nodeIntegration: true.

[MarshallOfSound]

@nornagon Removing nodeIntegration is not a breaking change given contextIsolation now defaults to true, AMD modules already won't work 😄

[nornagon]

Hm, wouldn't it still break contextIsolation: false, nodeIntegration: false? I believe in that configuration, this sort of thing is possible:

// preload.js
window.my_node_thing = require('some_native_module_or_whatever')

// index.html
<script>my_node_thing.whatever()</script>

Removing the ability to set nodeIntegration: false breaks this use case, which is I think not an uncommon one.

[pushkin-]

this is kind of a dupe of this

[nornagon]

Ah yeah, I forgot I made that! I'm going to close that one out as a dupe of this one, as I think a lot of the comments on that one are now outdated. @anaisbetts's comment at #15760 (comment) is interesting, I think we've effectively already done the nuking that she was concerned about there with the switch to contextIsolation: true / nodeIntegration: false by default. It doesn't seem to have been a total disaster?

Leaving aside the discussion about removing the nodeIntegration option entirely, I don't think adding sandbox: true as a default when nodeIntegration is false or unspecified will break much at all.

• if nodeIntegration is off, there's no way to require() a native module or perform filesystem actions that would otherwise be prevented by the sandbox. so adding the sandbox doesn't impose any additional restrictions here.
• if nodeIntegration is on, we won't switch on the sandbox unless explicitly requested, so apps depending on require('fs') in the renderer will continue to work, because they have to be specifying nodeIntegration: true anyway
• nodeIntegration: true has no effect when sandbox: true

The only issue that I can think of is on Linux distributions which don't enable unprivileged CLONE_NEWUSER (see #17972), apps which previously worked fine will start failing to boot unless they correctly configure the chrome-sandbox binary as suid root. See also #19550 for a proposed technique to ease this pain in development mode, since most packagers set up the helper correctly on install.

[vn971]

Can we please retain a way to disable sandboxing externally, via an environment variable?

The reason for that is that electron sandbox conflicts with external sandboxing. External sandoxing (via bubblewrap, firejail etc) can be aware of local user's preferences. For example, it might only give access to a white list of directories and forbid the rest.

[nornagon]

@vn971 that's a separate & unrelated discussion, I think, as this is already a concern with apps that internally enable the sandbox. See #15760 and #17972 for related discussions. tl;dr: --no-sandbox does what you're requesting.

[vn971]

@nornagon the problem is that not all apps respond to --no-sandbox if they have their own wrapper. An environment variable, on the other hand, is passed on to subprocesses, which is ideal (and only real working solution) for external sandboxes/jails.

[nornagon]

@vn971 please open a separate issue, let's keep this issue on the topic of enabling the sandbox option by default.

[nornagon]

The only issue that I can think of is on Linux distributions which don't enable unprivileged CLONE_NEWUSER (see #17972), apps which previously worked fine will start failing to boot unless they correctly configure the chrome-sandbox binary as suid root. See also #19550 for a proposed technique to ease this pain in development mode, since most packagers set up the helper correctly on install.

Thinking more about this, I don't think this is actually a breaking change in this regard either, as on Linux systems even without any BrowserWindows being created with sandbox: true, the GPU process is sandboxed by default, so this helper needs to be set up correctly already. So I don't think this is a concern.

[nornagon]

The plan now is to roll this out in a phased way, based on the value of preload and nodeIntegration. From #28496 (comment):

• If nodeIntegration is false or unspecified and preload is empty, enable sandbox by default.
  • feat: enable sandbox by default in limited circumstances #30197
  • Targeted for Electron 15 16
• If nodeIntegration is false or unspecified and preload is specified, and sandbox is unspecified, warn that the default will be changing.
  • Targeted for Electron 15 16
• If nodeIntegration is false or unspecified, enable sandbox by default.
  • Targeted for Electron 17 18

This gets us to a point where nodeIntegration: true implies sandbox: false, but sandbox: true will be the default in all other cases.