diff --git a/docs/api/browser-window.md b/docs/api/browser-window.md index 2fc0472d5936b..3bec2ea1b76c0 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) - 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 b8dd1fe37ca9d..cb13c0fe54899 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,14 +48,26 @@ 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 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) { + 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. 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..c8a9d2b1f3ea5 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,25 +121,83 @@ 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; + 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_) + 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_) + 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(); 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]); + 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 + 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 { 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 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 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> promise_; + bool world_safe_result_; CompletionCallback callback_; DISALLOW_COPY_AND_ASSIGN(ScriptExecutionCallback); @@ -492,10 +554,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 +620,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..4e5a04eecf118 100644 --- a/spec-main/api-web-frame-spec.ts +++ b/spec-main/api-web-frame-spec.ts @@ -2,12 +2,48 @@ 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.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, 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..160b37af7bf74 --- /dev/null +++ b/spec/fixtures/pages/world-safe-preload-error.js @@ -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); +}); 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); +});