From 92b9525cfd4eca5a3761f157ee8429915c96536f Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Tue, 22 Jan 2019 10:44:28 -0800 Subject: [PATCH] feat: enable mixed-sandbox mode by default (#15894) --- atom/app/atom_main_delegate.cc | 6 --- atom/browser/api/atom_api_app.cc | 16 +------ atom/browser/api/atom_api_app.h | 1 - atom/browser/atom_browser_client.cc | 11 +++++ atom/common/options_switches.cc | 3 -- atom/common/options_switches.h | 1 - docs/api/app.md | 6 --- docs/api/sandbox-option.md | 4 -- lib/browser/api/app.js | 3 ++ spec/api-app-spec.js | 50 --------------------- spec/api-browser-window-spec.js | 1 - spec/fixtures/api/mixed-sandbox-app/main.js | 4 -- 12 files changed, 15 insertions(+), 91 deletions(-) diff --git a/atom/app/atom_main_delegate.cc b/atom/app/atom_main_delegate.cc index 7cbe69e5a553..3dd19e84daae 100644 --- a/atom/app/atom_main_delegate.cc +++ b/atom/app/atom_main_delegate.cc @@ -238,12 +238,6 @@ void AtomMainDelegate::PreSandboxStartup() { // linux (namespace sandbox is available on most distros). command_line->AppendSwitch(service_manager::switches::kDisableSetuidSandbox); - if (!command_line->HasSwitch(switches::kEnableMixedSandbox) && - !command_line->HasSwitch(switches::kEnableSandbox)) { - // Disable renderer sandbox for most of node's functions. - command_line->AppendSwitch(service_manager::switches::kNoSandbox); - } - // Allow file:// URIs to read other file:// URIs by default. command_line->AppendSwitch(::switches::kAllowFileAccessFromFiles); diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index a81cc0356cd2..b959179e26e0 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -1250,19 +1250,6 @@ void App::EnableSandbox(mate::Arguments* args) { command_line->AppendSwitch(switches::kEnableSandbox); } -void App::EnableMixedSandbox(mate::Arguments* args) { - if (Browser::Get()->is_ready()) { - args->ThrowError( - "app.enableMixedSandbox() can only be called " - "before app is ready"); - return; - } - - auto* command_line = base::CommandLine::ForCurrentProcess(); - RemoveNoSandboxSwitch(command_line); - command_line->AppendSwitch(switches::kEnableMixedSandbox); -} - #if defined(OS_MACOSX) bool App::MoveToApplicationsFolder(mate::Arguments* args) { return ui::cocoa::AtomBundleMover::Move(args); @@ -1370,8 +1357,7 @@ void App::BuildPrototype(v8::Isolate* isolate, .SetMethod("startAccessingSecurityScopedResource", &App::StartAccessingSecurityScopedResource) #endif - .SetMethod("enableSandbox", &App::EnableSandbox) - .SetMethod("enableMixedSandbox", &App::EnableMixedSandbox); + .SetMethod("enableSandbox", &App::EnableSandbox); } } // namespace api diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index 94148f30a132..0fdd0200cc48 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -206,7 +206,6 @@ class App : public AtomBrowserClient::Delegate, v8::Local GetGPUInfo(v8::Isolate* isolate, const std::string& info_type); void EnableSandbox(mate::Arguments* args); - void EnableMixedSandbox(mate::Arguments* args); #if defined(OS_MACOSX) bool MoveToApplicationsFolder(mate::Arguments* args); diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc index 55bb7b020e34..7ff72c9dd7e7 100644 --- a/atom/browser/atom_browser_client.cc +++ b/atom/browser/atom_browser_client.cc @@ -71,6 +71,7 @@ #include "services/device/public/cpp/geolocation/location_provider.h" #include "services/network/public/cpp/resource_request_body.h" #include "services/proxy_resolver/public/mojom/proxy_resolver.mojom.h" +#include "services/service_manager/sandbox/switches.h" #include "ui/base/l10n/l10n_util.h" #include "ui/base/resource/resource_bundle.h" #include "v8/include/v8.h" @@ -504,6 +505,16 @@ void AtomBrowserClient::AppendExtraCommandLineSwitches( content::WebContents* web_contents = GetWebContentsFromProcessID(process_id); if (web_contents) { + // devtools processes must be launched unsandboxed in order for the remote + // API to work in devtools extensions. This is due to the fact that the + // remote API assumes that it will only be used from the main frame, but + // devtools extensions are loaded from an iframe. + // It would be possible to sandbox devtools extensions processes by default + // if we made the remote API work with multiple frames. + if (web_contents->GetVisibleURL().SchemeIs("chrome-devtools")) { + command_line->AppendSwitch(service_manager::switches::kNoSandbox); + command_line->AppendSwitch(::switches::kNoZygote); + } auto* web_preferences = WebContentsPreferences::From(web_contents); if (web_preferences) web_preferences->AppendCommandLineSwitches(command_line); diff --git a/atom/common/options_switches.cc b/atom/common/options_switches.cc index 9108b310c237..d1a5d0c7499d 100644 --- a/atom/common/options_switches.cc +++ b/atom/common/options_switches.cc @@ -161,9 +161,6 @@ namespace switches { // Enable chromium sandbox. const char kEnableSandbox[] = "enable-sandbox"; -// Enable sandbox in only remote content windows. -const char kEnableMixedSandbox[] = "enable-mixed-sandbox"; - // Enable plugins. const char kEnablePlugins[] = "enable-plugins"; diff --git a/atom/common/options_switches.h b/atom/common/options_switches.h index 4a8a40fe1e82..b2bdb8339e34 100644 --- a/atom/common/options_switches.h +++ b/atom/common/options_switches.h @@ -83,7 +83,6 @@ extern const char kOffscreen[]; namespace switches { extern const char kEnableSandbox[]; -extern const char kEnableMixedSandbox[]; extern const char kEnablePlugins[]; extern const char kPpapiFlashPath[]; extern const char kPpapiFlashVersion[]; diff --git a/docs/api/app.md b/docs/api/app.md index 9c9dbe121b4f..ca8b89b91206 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -1225,12 +1225,6 @@ Enables full sandbox mode on the app. This method can only be called before app is ready. -### `app.enableMixedSandbox()` _Experimental_ _macOS_ _Windows_ - -Enables mixed sandbox mode on the app. - -This method can only be called before app is ready. - ### `app.isInApplicationsFolder()` _macOS_ Returns `Boolean` - Whether the application is currently running from the diff --git a/docs/api/sandbox-option.md b/docs/api/sandbox-option.md index 1298eec5db38..e300849efc48 100644 --- a/docs/api/sandbox-option.md +++ b/docs/api/sandbox-option.md @@ -60,10 +60,6 @@ It is important to note that this option alone won't enable the OS-enforced sand `--enable-sandbox` command-line argument must be passed to electron, which will force `sandbox: true` for all `BrowserWindow` instances. -To enable OS-enforced sandbox on `BrowserWindow` or `webview` process with `sandbox:true` without causing -entire app to be in sandbox, `--enable-mixed-sandbox` command-line argument must be passed to electron. -This option is currently only supported on macOS and Windows. - ```js let win app.on('ready', () => { diff --git a/lib/browser/api/app.js b/lib/browser/api/app.js index 694ab2e59bb6..2c0695b6b18d 100644 --- a/lib/browser/api/app.js +++ b/lib/browser/api/app.js @@ -30,6 +30,9 @@ Object.assign(app, { getSwitchValue: (...args) => commandLine.getSwitchValue(...args.map(String)), appendSwitch: (...args) => commandLine.appendSwitch(...args.map(String)), appendArgument: (...args) => commandLine.appendArgument(...args.map(String)) + }, + enableMixedSandbox () { + deprecate.log(`'enableMixedSandbox' is deprecated. Mixed-sandbox mode is now enabled by default. You can safely remove the call to enableMixedSandbox().`) } }) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 02bed5aea8bf..d6c4e9d973e5 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -1111,56 +1111,6 @@ describe('app module', () => { }) }) }) - - describe('when app.enableMixedSandbox() is called', () => { - it('adds --enable-sandbox to renderer processes created with sandbox: true', done => { - const appPath = path.join(__dirname, 'fixtures', 'api', 'mixed-sandbox-app') - appProcess = ChildProcess.spawn(remote.process.execPath, [appPath, '--app-enable-mixed-sandbox']) - - server.once('error', error => { done(error) }) - - server.on('connection', client => { - client.once('data', data => { - const argv = JSON.parse(data) - expect(argv.sandbox).to.include('--enable-sandbox') - expect(argv.sandbox).to.not.include('--no-sandbox') - - expect(argv.noSandbox).to.not.include('--enable-sandbox') - expect(argv.noSandbox).to.include('--no-sandbox') - - expect(argv.noSandboxDevtools).to.be.true() - expect(argv.sandboxDevtools).to.be.true() - - done() - }) - }) - }) - }) - - describe('when the app is launched with --enable-mixed-sandbox', () => { - it('adds --enable-sandbox to renderer processes created with sandbox: true', done => { - const appPath = path.join(__dirname, 'fixtures', 'api', 'mixed-sandbox-app') - appProcess = ChildProcess.spawn(remote.process.execPath, [appPath, '--enable-mixed-sandbox']) - - server.once('error', error => { done(error) }) - - server.on('connection', client => { - client.once('data', data => { - const argv = JSON.parse(data) - expect(argv.sandbox).to.include('--enable-sandbox') - expect(argv.sandbox).to.not.include('--no-sandbox') - - expect(argv.noSandbox).to.not.include('--enable-sandbox') - expect(argv.noSandbox).to.include('--no-sandbox') - - expect(argv.noSandboxDevtools).to.be.true() - expect(argv.sandboxDevtools).to.be.true() - - done() - }) - }) - }) - }) }) describe('disableDomainBlockingFor3DAPIs() API', () => { diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index c0913ead05fb..1637c32993f3 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1873,7 +1873,6 @@ describe('BrowserWindow module', () => { it('validates process APIs access in sandboxed renderer', (done) => { ipcMain.once('answer', function (event, test) { - assert.strictEqual(test.pid, w.webContents.getOSProcessId()) assert.strictEqual(test.arch, remote.process.arch) assert.strictEqual(test.platform, remote.process.platform) assert.deepStrictEqual(...resolveGetters(test.env, remote.process.env)) diff --git a/spec/fixtures/api/mixed-sandbox-app/main.js b/spec/fixtures/api/mixed-sandbox-app/main.js index 1c2ea79fc604..0479ca1d7704 100644 --- a/spec/fixtures/api/mixed-sandbox-app/main.js +++ b/spec/fixtures/api/mixed-sandbox-app/main.js @@ -10,10 +10,6 @@ if (process.argv.includes('--app-enable-sandbox')) { app.enableSandbox() } -if (process.argv.includes('--app-enable-mixed-sandbox')) { - app.enableMixedSandbox() -} - let currentWindowSandboxed = false app.once('ready', () => {