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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 0 additions & 6 deletions atom/app/atom_main_delegate.cc
Expand Up @@ -204,12 +204,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);

Expand Down
16 changes: 1 addition & 15 deletions atom/browser/api/atom_api_app.cc
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion atom/browser/api/atom_api_app.h
Expand Up @@ -206,7 +206,6 @@ class App : public AtomBrowserClient::Delegate,
v8::Local<v8::Promise> 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);
Expand Down
11 changes: 11 additions & 0 deletions atom/browser/atom_browser_client.cc
Expand Up @@ -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"
Expand Down Expand Up @@ -497,6 +498,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);
Expand Down
3 changes: 0 additions & 3 deletions atom/common/options_switches.cc
Expand Up @@ -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";

Expand Down
1 change: 0 additions & 1 deletion atom/common/options_switches.h
Expand Up @@ -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[];
Expand Down
6 changes: 0 additions & 6 deletions docs/api/app.md
Expand Up @@ -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
Expand Down
4 changes: 0 additions & 4 deletions docs/api/sandbox-option.md
Expand Up @@ -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', () => {
Expand Down
3 changes: 3 additions & 0 deletions lib/browser/api/app.js
Expand Up @@ -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().`)
}
})

Expand Down
50 changes: 0 additions & 50 deletions spec/api-app-spec.js
Expand Up @@ -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', () => {
Expand Down
1 change: 0 additions & 1 deletion spec/api-browser-window-spec.js
Expand Up @@ -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))
Expand Down
4 changes: 0 additions & 4 deletions spec/fixtures/api/mixed-sandbox-app/main.js
Expand Up @@ -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', () => {
Expand Down