feat: add worldSafe flag for executeJS results (#24114)

* feat: add worldSafe flag for executeJS results

* chore: do not log warning for webContents.executeJS

* Apply suggestions from code review

Co-authored-by: Jeremy Rose <jeremya@chromium.org>

* chore: apply PR feedback

* chore: split logic a bit

* chore: allow primitives through the world safe checl

* chore: clean up per PR feedback

* chore: flip boolean logic

* chore: update per PR feedback

* chore: fix typo

* chore: fix spec

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
This commit is contained in:
Samuel Attard 2020-07-23 14:32:20 -07:00 committed by GitHub
parent 3b250b649b
commit b500294c1d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 160 additions and 7 deletions

View file

@ -348,6 +348,9 @@ It creates a new `BrowserWindow` with native properties as set by the `options`.
You can access this context in the dev tools by selecting the
'Electron Isolated Context' entry in the combo box at the top of the
Console tab.
* `worldSafeExecuteJavaScript` Boolean (optional) - If true, values returned from `webFrame.executeJavaScript` will be sanitized to ensure JS values
can't unsafely cross between worlds when using `contextIsolation`. The default
is `false`. In Electron 12, the default will be changed to `true`. _Deprecated_
* `nativeWindowOpen` Boolean (optional) - Whether to use native
`window.open()`. Defaults to `false`. Child windows will always have node
integration disabled unless `nodeIntegrationInSubFrames` is true. **Note:** This option is currently

View file

@ -1,4 +1,5 @@
import { EventEmitter } from 'events';
import deprecate from '@electron/internal/common/api/deprecate';
const binding = process._linkedBinding('electron_renderer_web_frame');
@ -47,14 +48,26 @@ class WebFrame extends EventEmitter {
}
}
const { hasSwitch } = process._linkedBinding('electron_common_command_line');
const worldSafeJS = hasSwitch('world-safe-execute-javascript') && hasSwitch('context-isolation');
// Populate the methods.
for (const name in binding) {
if (!name.startsWith('_')) { // some methods are manually populated above
// TODO(felixrieseberg): Once we can type web_frame natives, we could
// use a neat `keyof` here
(WebFrame as any).prototype[name] = function (...args: Array<any>) {
if (!worldSafeJS && name.startsWith('executeJavaScript')) {
deprecate.log(`Security Warning: webFrame.${name} was called without worldSafeExecuteJavaScript enabled. This is considered unsafe. worldSafeExecuteJavaScript will be enabled by default in Electron 12.`);
}
return binding[name](this.context, ...args);
};
// TODO(MarshallOfSound): Remove once the above deprecation is removed
if (name.startsWith('executeJavaScript')) {
(WebFrame as any).prototype[`_${name}`] = function (...args: Array<any>) {
return binding[name](this.context, ...args);
};
}
}
}

View file

@ -12,6 +12,11 @@ export const webFrameInit = () => {
ipcRendererUtils.handle('ELECTRON_INTERNAL_RENDERER_WEB_FRAME_METHOD', (
event, method: keyof WebFrameMethod, ...args: any[]
) => {
// TODO(MarshallOfSound): Remove once the world-safe-execute-javascript deprecation warning is removed
if (method.startsWith('executeJavaScript')) {
return (webFrame as any)[`_${method}`](...args);
}
// The TypeScript compiler cannot handle the sheer number of
// call signatures here and simply gives up. Incorrect invocations
// will be caught by "keyof WebFrameMethod" though.

View file

@ -137,6 +137,7 @@ WebContentsPreferences::WebContentsPreferences(
"electron");
}
SetDefaultBoolIfUndefined(options::kContextIsolation, false);
SetDefaultBoolIfUndefined(options::kWorldSafeExecuteJavaScript, false);
SetDefaultBoolIfUndefined(options::kJavaScript, true);
SetDefaultBoolIfUndefined(options::kImages, true);
SetDefaultBoolIfUndefined(options::kTextAreasAreResizable, true);
@ -351,6 +352,9 @@ void WebContentsPreferences::AppendCommandLineSwitches(
if (IsEnabled(options::kContextIsolation))
command_line->AppendSwitch(switches::kContextIsolation);
if (IsEnabled(options::kWorldSafeExecuteJavaScript))
command_line->AppendSwitch(switches::kWorldSafeExecuteJavaScript);
// --background-color.
std::string s;
if (GetAsString(&preference_, options::kBackgroundColor, &s)) {

View file

@ -114,6 +114,9 @@ const char kNodeIntegration[] = "nodeIntegration";
// Enable context isolation of Electron APIs and preload script
const char kContextIsolation[] = "contextIsolation";
// Enable world safe passing of values when using "executeJavaScript"
const char kWorldSafeExecuteJavaScript[] = "worldSafeExecuteJavaScript";
// Instance ID of guest WebContents.
const char kGuestInstanceID[] = "guestInstanceId";
@ -238,6 +241,7 @@ const char kPreloadScript[] = "preload";
const char kPreloadScripts[] = "preload-scripts";
const char kNodeIntegration[] = "node-integration";
const char kContextIsolation[] = "context-isolation";
const char kWorldSafeExecuteJavaScript[] = "world-safe-execute-javascript";
const char kGuestInstanceID[] = "guest-instance-id";
const char kOpenerID[] = "opener-id";
const char kScrollBounce[] = "scroll-bounce";

View file

@ -62,6 +62,7 @@ extern const char kPreloadScript[];
extern const char kPreloadURL[];
extern const char kNodeIntegration[];
extern const char kContextIsolation[];
extern const char kWorldSafeExecuteJavaScript[];
extern const char kGuestInstanceID[];
extern const char kExperimentalFeatures[];
extern const char kOpenerID[];
@ -121,6 +122,7 @@ extern const char kPreloadScript[];
extern const char kPreloadScripts[];
extern const char kNodeIntegration[];
extern const char kContextIsolation[];
extern const char kWorldSafeExecuteJavaScript[];
extern const char kGuestInstanceID[];
extern const char kOpenerID[];
extern const char kScrollBounce[];

View file

@ -21,6 +21,7 @@
#include "shell/common/gin_helper/error_thrower.h"
#include "shell/common/gin_helper/promise.h"
#include "shell/common/node_includes.h"
#include "shell/common/options_switches.h"
#include "shell/renderer/api/electron_api_spell_check_client.h"
#include "third_party/blink/public/common/page/page_zoom.h"
#include "third_party/blink/public/common/web_cache/web_cache_resource_type_stats.h"
@ -120,25 +121,83 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback {
explicit ScriptExecutionCallback(
gin_helper::Promise<v8::Local<v8::Value>> promise,
bool world_safe_result,
CompletionCallback callback)
: promise_(std::move(promise)), callback_(std::move(callback)) {}
: promise_(std::move(promise)),
world_safe_result_(world_safe_result),
callback_(std::move(callback)) {}
~ScriptExecutionCallback() override = default;
void CopyResultToCallingContextAndFinalize(
v8::Isolate* isolate,
const v8::Local<v8::Value>& result) {
blink::CloneableMessage ret;
bool success;
std::string error_message;
{
v8::TryCatch try_catch(isolate);
success = gin::ConvertFromV8(isolate, result, &ret);
if (try_catch.HasCaught()) {
auto message = try_catch.Message();
if (message.IsEmpty() ||
!gin::ConvertFromV8(isolate, message->Get(), &error_message)) {
error_message =
"An unknown exception occurred while getting the result of "
"the script";
}
}
}
if (!success) {
// Failed convert so we send undefined everywhere
if (callback_)
std::move(callback_).Run(
v8::Undefined(isolate),
v8::Exception::Error(
v8::String::NewFromUtf8(isolate, error_message.c_str())
.ToLocalChecked()));
promise_.RejectWithErrorMessage(error_message);
} else {
v8::Local<v8::Context> context = promise_.GetContext();
v8::Context::Scope context_scope(context);
v8::Local<v8::Value> cloned_value = gin::ConvertToV8(isolate, ret);
if (callback_)
std::move(callback_).Run(cloned_value, v8::Undefined(isolate));
promise_.Resolve(cloned_value);
}
}
void Completed(
const blink::WebVector<v8::Local<v8::Value>>& result) override {
v8::Isolate* isolate = v8::Isolate::GetCurrent();
if (!result.empty()) {
if (!result[0].IsEmpty()) {
v8::Local<v8::Value> value = result[0];
// Either world safe results are disabled or the result was created in
// the same world as the caller or the result is not an object and
// therefore does not have a prototype chain to protect
bool should_clone_value =
world_safe_result_ &&
!(value->IsObject() &&
promise_.GetContext() ==
value.As<v8::Object>()->CreationContext()) &&
value->IsObject();
if (should_clone_value) {
CopyResultToCallingContextAndFinalize(isolate, value);
} else {
// Right now only single results per frame is supported.
if (!callback_.is_null())
std::move(callback_).Run(result[0], v8::Undefined(isolate));
promise_.Resolve(result[0]);
if (callback_)
std::move(callback_).Run(value, v8::Undefined(isolate));
promise_.Resolve(value);
}
} else {
const char* error_message =
"Script failed to execute, this normally means an error "
"was thrown. Check the renderer console for the error.";
if (!callback_.is_null()) {
v8::Local<v8::Context> context = promise_.GetContext();
v8::Context::Scope context_scope(context);
std::move(callback_).Run(
v8::Undefined(isolate),
v8::Exception::Error(
@ -152,6 +211,8 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback {
"WebFrame was removed before script could run. This normally means "
"the underlying frame was destroyed";
if (!callback_.is_null()) {
v8::Local<v8::Context> context = promise_.GetContext();
v8::Context::Scope context_scope(context);
std::move(callback_).Run(
v8::Undefined(isolate),
v8::Exception::Error(v8::String::NewFromUtf8(isolate, error_message)
@ -164,6 +225,7 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback {
private:
gin_helper::Promise<v8::Local<v8::Value>> promise_;
bool world_safe_result_;
CompletionCallback callback_;
DISALLOW_COPY_AND_ASSIGN(ScriptExecutionCallback);
@ -491,10 +553,13 @@ v8::Local<v8::Promise> ExecuteJavaScript(gin_helper::Arguments* args,
ScriptExecutionCallback::CompletionCallback completion_callback;
args->GetNext(&completion_callback);
bool world_safe_exec_js = base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kWorldSafeExecuteJavaScript);
render_frame->GetWebFrame()->RequestExecuteScriptAndReturnValue(
blink::WebScriptSource(blink::WebString::FromUTF16(code)),
has_user_gesture,
new ScriptExecutionCallback(std::move(promise),
new ScriptExecutionCallback(std::move(promise), world_safe_exec_js,
std::move(completion_callback)));
return handle;
@ -554,13 +619,16 @@ v8::Local<v8::Promise> ExecuteJavaScriptInIsolatedWorld(
blink::WebURL(GURL(url)), start_line));
}
bool world_safe_exec_js = base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kWorldSafeExecuteJavaScript);
// Debugging tip: if you see a crash stack trace beginning from this call,
// then it is very likely that some exception happened when executing the
// "content_script/init.js" script.
render_frame->GetWebFrame()->RequestExecuteScriptInIsolatedWorld(
world_id, &sources.front(), sources.size(), has_user_gesture,
scriptExecutionType,
new ScriptExecutionCallback(std::move(promise),
new ScriptExecutionCallback(std::move(promise), world_safe_exec_js,
std::move(completion_callback)));
return handle;

View file

@ -2,12 +2,48 @@ import { expect } from 'chai';
import * as path from 'path';
import { BrowserWindow, ipcMain } from 'electron/main';
import { closeAllWindows } from './window-helpers';
import { emittedOnce } from './events-helpers';
describe('webFrame module', () => {
const fixtures = path.resolve(__dirname, '..', 'spec', 'fixtures');
afterEach(closeAllWindows);
for (const worldSafe of [true, false]) {
it(`can use executeJavaScript with world safe mode ${worldSafe ? 'enabled' : 'disabled'}`, async () => {
const w = new BrowserWindow({
show: true,
webPreferences: {
nodeIntegration: true,
contextIsolation: true,
worldSafeExecuteJavaScript: worldSafe,
preload: path.join(fixtures, 'pages', 'world-safe-preload.js')
}
});
const isSafe = emittedOnce(ipcMain, 'executejs-safe');
w.loadURL('about:blank');
const [, wasSafe] = await isSafe;
expect(wasSafe).to.equal(worldSafe);
});
}
it('can use executeJavaScript with world safe mode enabled and catch conversion errors', async () => {
const w = new BrowserWindow({
show: true,
webPreferences: {
nodeIntegration: true,
contextIsolation: true,
worldSafeExecuteJavaScript: true,
preload: path.join(fixtures, 'pages', 'world-safe-preload-error.js')
}
});
const execError = emittedOnce(ipcMain, 'executejs-safe');
w.loadURL('about:blank');
const [, error] = await execError;
expect(error).to.not.equal(null, 'Error should not be null');
expect(error).to.have.property('message', 'Uncaught Error: An object could not be cloned.');
});
it('calls a spellcheck provider', async () => {
const w = new BrowserWindow({
show: false,

View file

@ -0,0 +1,10 @@
const { ipcRenderer, webFrame } = require('electron');
webFrame.executeJavaScript(`(() => {
return Object(Symbol('a'));
})()`).catch((err) => {
// Considered safe if the object is constructed in this world
ipcRenderer.send('executejs-safe', err);
}).then(() => {
ipcRenderer.send('executejs-safe', null);
});

View file

@ -0,0 +1,8 @@
const { ipcRenderer, webFrame } = require('electron');
webFrame.executeJavaScript(`(() => {
return {};
})()`).then((obj) => {
// Considered safe if the object is constructed in this world
ipcRenderer.send('executejs-safe', obj.constructor === Object);
});