From 148769b15119ebf877dc2f0faabf083dc2ffc7f2 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Fri, 12 Jun 2020 15:50:50 -0700 Subject: [PATCH 01/12] feat: add worldSafe flag for executeJS results --- docs/api/browser-window.md | 3 ++ lib/renderer/api/web-frame.ts | 7 +++ shell/browser/web_contents_preferences.cc | 4 ++ shell/common/options_switches.cc | 4 ++ shell/common/options_switches.h | 2 + shell/renderer/api/electron_api_web_frame.cc | 49 +++++++++++++++++--- spec-main/api-web-frame-spec.ts | 19 ++++++++ spec/fixtures/pages/world-safe-preload.js | 8 ++++ 8 files changed, 89 insertions(+), 7 deletions(-) create mode 100644 spec/fixtures/pages/world-safe-preload.js diff --git a/docs/api/browser-window.md b/docs/api/browser-window.md index 2fc0472d5936b..0e404fd229abb 100644 --- a/docs/api/browser-window.md +++ b/docs/api/browser-window.md @@ -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) - Whether to ensure JS values + can't unsafely cross worlds when using `webFrame.executeJavaScript`. The default + is `false` though this will be irreversibly be changing to `true` in Electron 12. * `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 diff --git a/lib/renderer/api/web-frame.ts b/lib/renderer/api/web-frame.ts index b8dd1fe37ca9d..d119717037eab 100644 --- a/lib/renderer/api/web-frame.ts +++ b/lib/renderer/api/web-frame.ts @@ -1,4 +1,5 @@ import { EventEmitter } from 'events'; +import deprecate from '@electron/internal/common/api/deprecate'; const binding = process.electronBinding('web_frame'); @@ -47,12 +48,18 @@ class WebFrame extends EventEmitter { } } +const { hasSwitch } = process.electronBinding('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) { + if (!worldSafeJS && name.startsWith('executeJavaScript')) { + deprecate.log(`Security Warning: webFrame.${name} was called without worldSafeExecuteJavaScript set to true. This is considered unsafe and the default of worldSafeExecuteJavaScript will be changing to true in Electron 12.`); + } return binding[name](this.context, ...args); }; } diff --git a/shell/browser/web_contents_preferences.cc b/shell/browser/web_contents_preferences.cc index 4fffe4cc4e0b8..e8bd2ee1ed6ce 100644 --- a/shell/browser/web_contents_preferences.cc +++ b/shell/browser/web_contents_preferences.cc @@ -127,6 +127,7 @@ WebContentsPreferences::WebContentsPreferences( SetDefaultBoolIfUndefined(options::kSandbox, false); SetDefaultBoolIfUndefined(options::kNativeWindowOpen, false); SetDefaultBoolIfUndefined(options::kContextIsolation, false); + SetDefaultBoolIfUndefined(options::kWorldSafeExecuteJavaScript, false); SetDefaultBoolIfUndefined(options::kJavaScript, true); SetDefaultBoolIfUndefined(options::kImages, true); SetDefaultBoolIfUndefined(options::kTextAreasAreResizable, true); @@ -337,6 +338,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)) { diff --git a/shell/common/options_switches.cc b/shell/common/options_switches.cc index a398639aa32a9..c4561f0cb5fd8 100644 --- a/shell/common/options_switches.cc +++ b/shell/common/options_switches.cc @@ -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"; @@ -235,6 +238,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"; diff --git a/shell/common/options_switches.h b/shell/common/options_switches.h index 6496a9fbe6f5f..1c7555d79cc38 100644 --- a/shell/common/options_switches.h +++ b/shell/common/options_switches.h @@ -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[]; @@ -120,6 +121,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[]; diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index d1fd622391d8f..9134eda734487 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -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,8 +121,11 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { explicit ScriptExecutionCallback( gin_helper::Promise> 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; @@ -130,10 +134,34 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { v8::Isolate* isolate = v8::Isolate::GetCurrent(); if (!result.empty()) { if (!result[0].IsEmpty()) { - // 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 (!world_safe_result_) { + // 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]); + } else { + // Serializable objects + blink::CloneableMessage ret; + bool success; + { + v8::TryCatch try_catch(isolate); + success = gin::ConvertFromV8(isolate, result[0], &ret); + } + if (!success) { + // Failed convert so we send undefined everywhere + if (!callback_.is_null()) + std::move(callback_).Run(v8::Undefined(isolate), + v8::Undefined(isolate)); + promise_.Resolve(v8::Undefined(isolate)); + } else { + v8::Local context = promise_.GetContext(); + v8::Context::Scope context_scope(context); + v8::Local cloned_value = gin::ConvertToV8(isolate, ret); + if (!callback_.is_null()) + std::move(callback_).Run(cloned_value, v8::Undefined(isolate)); + promise_.Resolve(cloned_value); + } + } } else { const char* error_message = "Script failed to execute, this normally means an error " @@ -164,6 +192,7 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { private: gin_helper::Promise> promise_; + bool world_safe_result_; CompletionCallback callback_; DISALLOW_COPY_AND_ASSIGN(ScriptExecutionCallback); @@ -492,10 +521,13 @@ v8::Local 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; @@ -555,13 +587,16 @@ v8::Local 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; diff --git a/spec-main/api-web-frame-spec.ts b/spec-main/api-web-frame-spec.ts index 75ad0bda69b4f..22c0b39745f72 100644 --- a/spec-main/api-web-frame-spec.ts +++ b/spec-main/api-web-frame-spec.ts @@ -2,12 +2,31 @@ import { expect } from 'chai'; import * as path from 'path'; import { BrowserWindow, ipcMain } from 'electron'; 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.loadFile(path.join(fixtures, 'pages', 'blank.html')); + const [, wasSafe] = await isSafe; + expect(wasSafe).to.equal(worldSafe); + }); + } + it('calls a spellcheck provider', async () => { const w = new BrowserWindow({ show: false, diff --git a/spec/fixtures/pages/world-safe-preload.js b/spec/fixtures/pages/world-safe-preload.js new file mode 100644 index 0000000000000..32f8be31bc878 --- /dev/null +++ b/spec/fixtures/pages/world-safe-preload.js @@ -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); +}); From 91e48df7d96c2ba5abb1b2d6a0a7daa240607e7f Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 15 Jun 2020 14:30:00 -0700 Subject: [PATCH 02/12] chore: do not log warning for webContents.executeJS --- lib/renderer/api/web-frame.ts | 6 ++++++ lib/renderer/web-frame-init.ts | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/lib/renderer/api/web-frame.ts b/lib/renderer/api/web-frame.ts index d119717037eab..de5c0206e935f 100644 --- a/lib/renderer/api/web-frame.ts +++ b/lib/renderer/api/web-frame.ts @@ -62,6 +62,12 @@ for (const name in binding) { } 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) { + return binding[name](this.context, ...args); + }; + } } } diff --git a/lib/renderer/web-frame-init.ts b/lib/renderer/web-frame-init.ts index b4e8604b05b13..86edc0d93c13e 100644 --- a/lib/renderer/web-frame-init.ts +++ b/lib/renderer/web-frame-init.ts @@ -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. From 0e7edca6d1c2717b41978338538b1275d269c8ea Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 24 Jun 2020 09:23:29 -0700 Subject: [PATCH 03/12] Apply suggestions from code review Co-authored-by: Jeremy Rose --- docs/api/browser-window.md | 6 +++--- lib/renderer/api/web-frame.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/api/browser-window.md b/docs/api/browser-window.md index 0e404fd229abb..3bec2ea1b76c0 100644 --- a/docs/api/browser-window.md +++ b/docs/api/browser-window.md @@ -348,9 +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) - Whether to ensure JS values - can't unsafely cross worlds when using `webFrame.executeJavaScript`. The default - is `false` though this will be irreversibly be changing to `true` in Electron 12. + * `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 diff --git a/lib/renderer/api/web-frame.ts b/lib/renderer/api/web-frame.ts index de5c0206e935f..cb13c0fe54899 100644 --- a/lib/renderer/api/web-frame.ts +++ b/lib/renderer/api/web-frame.ts @@ -58,7 +58,7 @@ for (const name in binding) { // use a neat `keyof` here (WebFrame as any).prototype[name] = function (...args: Array) { if (!worldSafeJS && name.startsWith('executeJavaScript')) { - deprecate.log(`Security Warning: webFrame.${name} was called without worldSafeExecuteJavaScript set to true. This is considered unsafe and the default of worldSafeExecuteJavaScript will be changing to true in Electron 12.`); + 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); }; From 842b0ac675b5b5e40a33005adadedcf1dc77da76 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 24 Jun 2020 09:50:29 -0700 Subject: [PATCH 04/12] chore: apply PR feedback --- lib/renderer/api/web-frame.ts | 2 +- shell/renderer/api/electron_api_web_frame.cc | 28 ++++++++++++++++--- spec-main/api-web-frame-spec.ts | 18 +++++++++++- .../pages/world-safe-preload-error.js | 8 ++++++ 4 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 spec/fixtures/pages/world-safe-preload-error.js diff --git a/lib/renderer/api/web-frame.ts b/lib/renderer/api/web-frame.ts index cb13c0fe54899..550fe72853cd2 100644 --- a/lib/renderer/api/web-frame.ts +++ b/lib/renderer/api/web-frame.ts @@ -48,7 +48,7 @@ class WebFrame extends EventEmitter { } } -const { hasSwitch } = process.electronBinding('command_line'); +const { hasSwitch } = process._linkedBinding('electron_common_command_line'); const worldSafeJS = hasSwitch('world-safe-execute-javascript') && hasSwitch('context-isolation'); // Populate the methods. diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index 9134eda734487..d8736af77e387 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -134,7 +134,12 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { v8::Isolate* isolate = v8::Isolate::GetCurrent(); if (!result.empty()) { if (!result[0].IsEmpty()) { - if (!world_safe_result_) { + // Either world safe results are disabled or the result was created in + // the same world as the caller + if (!world_safe_result_ || + (result[0]->IsObject() && + promise_.GetContext() == + result[0].As()->CreationContext())) { // Right now only single results per frame is supported. if (!callback_.is_null()) std::move(callback_).Run(result[0], v8::Undefined(isolate)); @@ -143,16 +148,31 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { // Serializable objects blink::CloneableMessage ret; bool success; + std::string error_message; { v8::TryCatch try_catch(isolate); success = gin::ConvertFromV8(isolate, result[0], &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_.is_null()) - std::move(callback_).Run(v8::Undefined(isolate), - v8::Undefined(isolate)); - promise_.Resolve(v8::Undefined(isolate)); + 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 context = promise_.GetContext(); v8::Context::Scope context_scope(context); diff --git a/spec-main/api-web-frame-spec.ts b/spec-main/api-web-frame-spec.ts index 22c0b39745f72..268dbac6202c1 100644 --- a/spec-main/api-web-frame-spec.ts +++ b/spec-main/api-web-frame-spec.ts @@ -21,12 +21,28 @@ describe('webFrame module', () => { } }); const isSafe = emittedOnce(ipcMain, 'executejs-safe'); - w.loadFile(path.join(fixtures, 'pages', 'blank.html')); + 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.have.property('message', 'Uncaught Error: An object could not be cloned.'); + }); + it('calls a spellcheck provider', async () => { const w = new BrowserWindow({ show: false, diff --git a/spec/fixtures/pages/world-safe-preload-error.js b/spec/fixtures/pages/world-safe-preload-error.js new file mode 100644 index 0000000000000..23d5c119eb3f7 --- /dev/null +++ b/spec/fixtures/pages/world-safe-preload-error.js @@ -0,0 +1,8 @@ +const { ipcRenderer, webFrame } = require('electron'); + +webFrame.executeJavaScript(`(() => { + return Symbol('a'); +})()`).catch((err) => { + // Considered safe if the object is constructed in this world + ipcRenderer.send('executejs-safe', err); +}); From 8de20578c71bf36ae8bd63d7cf2058051c372e17 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Thu, 25 Jun 2020 09:42:08 -0700 Subject: [PATCH 05/12] chore: split logic a bit --- shell/renderer/api/electron_api_web_frame.cc | 76 ++++++++++---------- 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index d8736af77e387..22a77f2a74165 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -129,6 +129,45 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { ~ScriptExecutionCallback() override = default; + void CopyResultToCallingContextAndFinalize( + v8::Isolate* isolate, + const v8::Local& 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_.is_null()) + 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 context = promise_.GetContext(); + v8::Context::Scope context_scope(context); + v8::Local cloned_value = gin::ConvertToV8(isolate, ret); + if (!callback_.is_null()) + std::move(callback_).Run(cloned_value, v8::Undefined(isolate)); + promise_.Resolve(cloned_value); + } + } + void Completed( const blink::WebVector>& result) override { v8::Isolate* isolate = v8::Isolate::GetCurrent(); @@ -145,42 +184,7 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { std::move(callback_).Run(result[0], v8::Undefined(isolate)); promise_.Resolve(result[0]); } else { - // Serializable objects - blink::CloneableMessage ret; - bool success; - std::string error_message; - { - v8::TryCatch try_catch(isolate); - success = gin::ConvertFromV8(isolate, result[0], &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_.is_null()) - 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 context = promise_.GetContext(); - v8::Context::Scope context_scope(context); - v8::Local cloned_value = gin::ConvertToV8(isolate, ret); - if (!callback_.is_null()) - std::move(callback_).Run(cloned_value, v8::Undefined(isolate)); - promise_.Resolve(cloned_value); - } + CopyResultToCallingContextAndFinalize(isolate, result[0]); } } else { const char* error_message = From 2ea5b136b4dad14c2a913cfe4975a3dd1ad85e18 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 13 Jul 2020 11:38:21 -0700 Subject: [PATCH 06/12] chore: allow primitives through the world safe checl --- shell/renderer/api/electron_api_web_frame.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index 22a77f2a74165..59827212b3b19 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -174,11 +174,13 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { if (!result.empty()) { if (!result[0].IsEmpty()) { // Either world safe results are disabled or the result was created in - // the same world as the caller + // the same world as the caller or the result is not an object and + // therefore does not have a prototype chain to protect if (!world_safe_result_ || (result[0]->IsObject() && promise_.GetContext() == - result[0].As()->CreationContext())) { + result[0].As()->CreationContext()) || + !result[0]->IsObject()) { // Right now only single results per frame is supported. if (!callback_.is_null()) std::move(callback_).Run(result[0], v8::Undefined(isolate)); From 958866bea52d06467d3959dcb13da71ce5ce5b1b Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 13 Jul 2020 14:52:27 -0700 Subject: [PATCH 07/12] chore: clean up per PR feedback --- shell/renderer/api/electron_api_web_frame.cc | 23 +++++++++++--------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index 59827212b3b19..6b60a1b2b98c6 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -151,7 +151,7 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { } if (!success) { // Failed convert so we send undefined everywhere - if (!callback_.is_null()) + if (callback_) std::move(callback_).Run( v8::Undefined(isolate), v8::Exception::Error( @@ -162,7 +162,7 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { v8::Local context = promise_.GetContext(); v8::Context::Scope context_scope(context); v8::Local cloned_value = gin::ConvertToV8(isolate, ret); - if (!callback_.is_null()) + if (callback_) std::move(callback_).Run(cloned_value, v8::Undefined(isolate)); promise_.Resolve(cloned_value); } @@ -173,20 +173,23 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { v8::Isolate* isolate = v8::Isolate::GetCurrent(); if (!result.empty()) { if (!result[0].IsEmpty()) { + v8::Local value = reuslt[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 - if (!world_safe_result_ || - (result[0]->IsObject() && + bool should_send_directly = + !world_safe_result_ || + (value->IsObject() && promise_.GetContext() == - result[0].As()->CreationContext()) || - !result[0]->IsObject()) { + value.As()->CreationContext()) || + !value->IsObject(); + if (should_send_directly) { // 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 { - CopyResultToCallingContextAndFinalize(isolate, result[0]); + CopyResultToCallingContextAndFinalize(isolate, value); } } else { const char* error_message = From e6cef33f3cc29e40821a94ed2bff55243bb330c8 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 15 Jul 2020 09:46:16 -0700 Subject: [PATCH 08/12] chore: flip boolean logic --- shell/renderer/api/electron_api_web_frame.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index 6b60a1b2b98c6..5c3bd7ea47e44 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -177,19 +177,19 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { // 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_send_directly = - !world_safe_result_ || - (value->IsObject() && - promise_.GetContext() == - value.As()->CreationContext()) || - !value->IsObject(); - if (should_send_directly) { + bool should_clone_value = + world_safe_result_ && + !(value->IsObject() && + promise_.GetContext() == + value.As()->CreationContext()) && + value->IsObject() if (should_clone_value) { + CopyResultToCallingContextAndFinalize(isolate, value); + } + else { // Right now only single results per frame is supported. if (callback_) std::move(callback_).Run(value, v8::Undefined(isolate)); promise_.Resolve(value); - } else { - CopyResultToCallingContextAndFinalize(isolate, value); } } else { const char* error_message = From cfb98e8557e0805c997bacabcab18ca7586f745d Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Thu, 16 Jul 2020 11:44:54 -0700 Subject: [PATCH 09/12] chore: update per PR feedback --- shell/renderer/api/electron_api_web_frame.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index 5c3bd7ea47e44..be272e7d720d7 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -182,10 +182,10 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { !(value->IsObject() && promise_.GetContext() == value.As()->CreationContext()) && - value->IsObject() if (should_clone_value) { + value->IsObject(); + if (should_clone_value) { CopyResultToCallingContextAndFinalize(isolate, value); - } - else { + } else { // Right now only single results per frame is supported. if (callback_) std::move(callback_).Run(value, v8::Undefined(isolate)); @@ -196,6 +196,8 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { "Script failed to execute, this normally means an error " "was thrown. Check the renderer console for the error."; if (!callback_.is_null()) { + v8::Local context = promise_.GetContext(); + v8::Context::Scope context_scope(context); std::move(callback_).Run( v8::Undefined(isolate), v8::Exception::Error( @@ -209,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 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) From 5d5d0ee6b971771ad1b0c08a2d29dccaace79c1c Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Thu, 16 Jul 2020 16:25:10 -0700 Subject: [PATCH 10/12] chore: fix typo --- shell/renderer/api/electron_api_web_frame.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index be272e7d720d7..c8a9d2b1f3ea5 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -173,7 +173,7 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { v8::Isolate* isolate = v8::Isolate::GetCurrent(); if (!result.empty()) { if (!result[0].IsEmpty()) { - v8::Local value = reuslt[0]; + v8::Local 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 From 801fc54cede2f2fc1cf4515f2745376c52cd0aca Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 20 Jul 2020 12:48:41 -0700 Subject: [PATCH 11/12] chore: fix spec --- spec-main/api-web-frame-spec.ts | 1 + spec/fixtures/pages/world-safe-preload-error.js | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/spec-main/api-web-frame-spec.ts b/spec-main/api-web-frame-spec.ts index 268dbac6202c1..4e5a04eecf118 100644 --- a/spec-main/api-web-frame-spec.ts +++ b/spec-main/api-web-frame-spec.ts @@ -40,6 +40,7 @@ describe('webFrame module', () => { 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.'); }); diff --git a/spec/fixtures/pages/world-safe-preload-error.js b/spec/fixtures/pages/world-safe-preload-error.js index 23d5c119eb3f7..160b37af7bf74 100644 --- a/spec/fixtures/pages/world-safe-preload-error.js +++ b/spec/fixtures/pages/world-safe-preload-error.js @@ -1,8 +1,10 @@ const { ipcRenderer, webFrame } = require('electron'); webFrame.executeJavaScript(`(() => { - return Symbol('a'); + 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); }); From e883f240de0d85d8a88a522806df9b95323246dc Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Thu, 23 Jul 2020 15:36:22 -0700 Subject: [PATCH 12/12] Update web-frame.ts --- lib/renderer/api/web-frame.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/renderer/api/web-frame.ts b/lib/renderer/api/web-frame.ts index 550fe72853cd2..cb13c0fe54899 100644 --- a/lib/renderer/api/web-frame.ts +++ b/lib/renderer/api/web-frame.ts @@ -48,7 +48,7 @@ class WebFrame extends EventEmitter { } } -const { hasSwitch } = process._linkedBinding('electron_common_command_line'); +const { hasSwitch } = process.electronBinding('command_line'); const worldSafeJS = hasSwitch('world-safe-execute-javascript') && hasSwitch('context-isolation'); // Populate the methods.