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

feat: enable mixed-sandbox mode by default #15894

Merged
merged 6 commits into from Jan 22, 2019

Conversation

Projects
None yet
4 participants
@nornagon
Copy link
Contributor

commented Nov 29, 2018

Description of Change

Currently, in order to launch some renderers sandboxed, an app must enable mixed-sandbox mode either with the command-line switch --enable-mixed-sandbox or the app.enableMixedSandbox() API. This PR will deprecate that switch, and make mixed-sandbox mode the default. Renderers launched with sandbox: true will now be actually sandboxed, where previously they would only be sandboxed if mixed-sandbox mode was also enabled.

Depends on #15870 for mixed-sandbox mode to be meaningful on Linux.

Checklist

Release Notes

Notes: Mixed-sandbox mode is now enabled by default. enableMixedSandbox and the --enable-mixed-sandbox command-line switch still exist for compatibility, but are deprecated and have no effect.

@nornagon nornagon requested review from as code owners Nov 29, 2018

@miniak

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2018

Be aware that

  • GPU process is going to be sandboxed, this should not have any negative effect on apps
  • pepper plugin host processes are going to be sandboxed, this might be a breaking change if apps depend on so-called host-native pepper plugins (should be possible to add an option to opt-out of sandbox for particular plugin / MIME type maybe)

cc @nornagon

@miniak

miniak approved these changes Dec 3, 2018

@nornagon nornagon referenced this pull request Dec 3, 2018

Merged

chore: enable v2 sandbox on mac #15647

2 of 5 tasks complete

@nornagon nornagon force-pushed the enable-mixed-sandbox branch from 3f9eaa3 to 7421cc3 Dec 3, 2018

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

Additionally, devtools processes will be sandboxed.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

@nornagon will that break devtron?

@zcbenz
Copy link
Member

left a comment

@nornagon
After enabling sandbox mode for devtools process, the devtools extension APIs implementation would switch to use remote for Node.js builtin modules instead of using fs module directly.

However currently remote is not fully supported in devtools process. The devtools extensions are loaded as iframes, so there would be multiple frames using remote module in one process, but the current model of remote assumes only the main frame would use remote module. This results in devtools extensions not working correctly with mixed sandbox mode.

I tried to modify remote to be friendly with multi-frames process, but it turns out to be a harder problem than I expect.

I propose disabling sandbox for devtools process in this PR for now, and enable it after we made remote capable of handling multi-frames process. Otherwise this PR would be blocked for quite a while.

@nornagon nornagon force-pushed the enable-mixed-sandbox branch from 7421cc3 to e7d4d4d Dec 6, 2018

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

Pushed a change which unsandboxes devtools processes.

@zcbenz

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

@nornagon The Linux CI are still failing for devtools tests, it seems that sandbox are still enabled for devtools process because of zygote process?

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

As mentioned in the PR description, this depends on #15870, i think i just need to rebase

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

hm, actually it looks like that PR is already merged in the base of this PR. I'll investigate.

@nornagon nornagon force-pushed the enable-mixed-sandbox branch from e7d4d4d to 19e16f7 Jan 18, 2019

@nornagon nornagon requested review from as code owners Jan 18, 2019

@nornagon nornagon force-pushed the enable-mixed-sandbox branch from 19e16f7 to b597fd9 Jan 18, 2019

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

@zcbenz linux tests are now passing; i think the ARM failures are unrelated

@zcbenz

zcbenz approved these changes Jan 21, 2019

Copy link
Member

left a comment

Looks good to me.

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

Talked with @miniak and agreed that we can follow up on the ppapi sandboxing stuff during the beta cycle.

@nornagon nornagon merged commit 92b9525 into master Jan 22, 2019

23 of 27 checks passed

appveyor: win-ia32-testing AppVeyor build failed
Details
electron-arm-testing Build #20190118.9 has failed
Details
electron-arm64-testing Build #20190118.9 has failed
Details
Artifact Comparison Changes Detected
Details
Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
electron-lint Build #20190118.9 succeeded
Details
electron-mas-testing Build #20190121.4 succeeded
Details
electron-osx-testing Build #20190118.11 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Jan 22, 2019

Release Notes Persisted

Mixed-sandbox mode is now enabled by default. enableMixedSandbox and the --enable-mixed-sandbox command-line switch still exist for compatibility, but are deprecated and have no effect.

@nornagon nornagon deleted the enable-mixed-sandbox branch Jan 22, 2019

@miniak miniak referenced this pull request Mar 19, 2019

Merged

docs: update sandbox-option.md #17468

4 of 4 tasks complete

@miniak miniak referenced this pull request Apr 21, 2019

Merged

chore: remove deprecated app.enableMixedSandbox() #17894

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.