From 6e49e976d91e3e7a6e0d1a0072e5ce193429354f Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Fri, 12 Jun 2020 15:50:50 -0700 Subject: [PATCH 01/11] 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 abe42fb8de5d8..c706b1226aa23 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 2f9da2435a673..08bf794a4a7d1 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._linkedBinding('electron_renderer_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 464181c9051e8..91b170eebe831 100644 --- a/shell/browser/web_contents_preferences.cc +++ b/shell/browser/web_contents_preferences.cc @@ -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)) { diff --git a/shell/common/options_switches.cc b/shell/common/options_switches.cc index b0483e4a64f42..bfeb9fee01b11 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"; @@ -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"; diff --git a/shell/common/options_switches.h b/shell/common/options_switches.h index e58b9ef7f6f2c..9eb5b43970b9d 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[]; @@ -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[]; diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index 0a298e7d4d5c4..4cb622e578517 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); @@ -491,10 +520,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; @@ -554,13 +586,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 6cafbf4146404..08c923989addd 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/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.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 7d92ae58e638dbfeb14bc35ec8396d50e4c06812 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 15 Jun 2020 14:30:00 -0700 Subject: [PATCH 02/11] 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 08bf794a4a7d1..fa4026003a6ce 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 e105af3b9db0421af66e74094785d24338f0f851 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 24 Jun 2020 09:23:29 -0700 Subject: [PATCH 03/11] 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 c706b1226aa23..b878d555f137b 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 fa4026003a6ce..2700895d5859e 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 4a365626bdc8e01e2a5b8fc9d5045c230f3e74fa Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 24 Jun 2020 09:50:29 -0700 Subject: [PATCH 04/11] 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 2700895d5859e..919d8921d4262 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 4cb622e578517..9641f5bb014e9 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 08c923989addd..e6e29aa7366b3 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 cf963824e8c1ffe64615d750a68a83abfdadcd6d Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Thu, 25 Jun 2020 09:42:08 -0700 Subject: [PATCH 05/11] 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 9641f5bb014e9..07d29265a217b 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 96158194d2647dd036c1975380fb957aaf5b1f74 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 13 Jul 2020 11:38:21 -0700 Subject: [PATCH 06/11] 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 07d29265a217b..d34bf868611df 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 fa51bf89088fc14c0af16af41e7e4f9af8e7e117 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 13 Jul 2020 14:52:27 -0700 Subject: [PATCH 07/11] 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 d34bf868611df..98fccb9f3b70c 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 42162d27a07fe1bfdd35f96ee4e933e38156d7ab Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 15 Jul 2020 09:46:16 -0700 Subject: [PATCH 08/11] 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 98fccb9f3b70c..ee20e8a7a172a 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 e407449ccf7217fe9b38681931e341a0b621193b Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Thu, 16 Jul 2020 11:44:54 -0700 Subject: [PATCH 09/11] 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 ee20e8a7a172a..c96742c3ec5eb 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 b82a906634cc52150eda797960db7ba86dff7fd2 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Thu, 16 Jul 2020 16:25:10 -0700 Subject: [PATCH 10/11] 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 c96742c3ec5eb..a1f0a2702c7fd 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 149b3bac106b0a1cbbef716818d77450cfbc0f78 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 20 Jul 2020 12:48:41 -0700 Subject: [PATCH 11/11] 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 e6e29aa7366b3..6c96901316984 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); });