From db08f08b88c7dca7b03a6d6ebe1e33be1cb473c9 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 26 Jan 2021 14:23:35 -0800 Subject: [PATCH] feat: enable world safe JS by default (#26889) * feat: enable world safe JS by default * refactor: use the ctx bridge to send executeJavaScript results in a world safe way * docs: add more info about the breaking change * include default in IsEnabled check --- docs/breaking-changes.md | 19 ++++++++++++ shell/browser/web_contents_preferences.cc | 4 +-- .../api/electron_api_context_bridge.cc | 30 ++++++++---------- .../api/electron_api_context_bridge.h | 25 +++++++++++++++ shell/renderer/api/electron_api_web_frame.cc | 31 ++++++++++++------- spec-main/api-context-bridge-spec.ts | 14 +++++++-- 6 files changed, 90 insertions(+), 33 deletions(-) diff --git a/docs/breaking-changes.md b/docs/breaking-changes.md index daaef94455..b78a915b2d 100644 --- a/docs/breaking-changes.md +++ b/docs/breaking-changes.md @@ -12,6 +12,16 @@ This document uses the following convention to categorize breaking changes: * **Deprecated:** An API was marked as deprecated. The API will continue to function, but will emit a deprecation warning, and will be removed in a future release. * **Removed:** An API or feature was removed, and is no longer supported by Electron. +## Planned Breaking API Changes (14.0) + +### Removed: `worldSafeExecuteJavaScript` + +In Electron 14, `worldSafeExecuteJavaScript` will be removed. There is no alternative, please +ensure your code works with this property enabled. It has been enabled by default since Electron +12. + +You will be affected by this change if you use either `webFrame.executeJavaScript` or `webFrame.executeJavaScriptInIsolatedWorld`. You will need to ensure that values returned by either of those methods are supported by the [Context Bridge API](api/context-bridge.md#parameter--error--return-type-support) as these methods use the same value passing semantics. + ## Planned Breaking API Changes (13.0) ### API Changed: `session.setPermissionCheckHandler(handler)` @@ -128,6 +138,15 @@ Chromium has removed support for Flash, and so we must follow suit. See Chromium's [Flash Roadmap](https://www.chromium.org/flash-roadmap) for more details. +### Default Changed: `worldSafeExecuteJavaScript` defaults to `true` + +In Electron 12, `worldSafeExecuteJavaScript` will be enabled by default. To restore +the previous behavior, `worldSafeExecuteJavaScript: false` must be specified in WebPreferences. +Please note that setting this option to `false` is **insecure**. + +This option will be removed in Electron 14 so please migrate your code to support the default +value. + ### Default Changed: `contextIsolation` defaults to `true` In Electron 12, `contextIsolation` will be enabled by default. To restore diff --git a/shell/browser/web_contents_preferences.cc b/shell/browser/web_contents_preferences.cc index 3561997a7a..f72400479b 100644 --- a/shell/browser/web_contents_preferences.cc +++ b/shell/browser/web_contents_preferences.cc @@ -137,7 +137,7 @@ WebContentsPreferences::WebContentsPreferences( "electron"); } SetDefaultBoolIfUndefined(options::kContextIsolation, false); - SetDefaultBoolIfUndefined(options::kWorldSafeExecuteJavaScript, false); + SetDefaultBoolIfUndefined(options::kWorldSafeExecuteJavaScript, true); SetDefaultBoolIfUndefined(options::kJavaScript, true); SetDefaultBoolIfUndefined(options::kImages, true); SetDefaultBoolIfUndefined(options::kTextAreasAreResizable, true); @@ -437,7 +437,7 @@ void WebContentsPreferences::OverrideWebkitPrefs( #endif prefs->world_safe_execute_javascript = - IsEnabled(options::kWorldSafeExecuteJavaScript); + IsEnabled(options::kWorldSafeExecuteJavaScript, true); int guest_instance_id = 0; if (GetAsInteger(&preference_, options::kGuestInstanceID, &guest_instance_id)) diff --git a/shell/renderer/api/electron_api_context_bridge.cc b/shell/renderer/api/electron_api_context_bridge.cc index 1414cc09f0..99616cf91b 100644 --- a/shell/renderer/api/electron_api_context_bridge.cc +++ b/shell/renderer/api/electron_api_context_bridge.cc @@ -128,22 +128,6 @@ v8::MaybeLocal GetPrivate(v8::Local context, gin::StringToV8(context->GetIsolate(), key))); } -// Where the context bridge should create the exception it is about to throw -enum class BridgeErrorTarget { - // The source / calling context. This is default and correct 99% of the time, - // the caller / context asking for the conversion will receive the error and - // therefore the error should be made in that context - kSource, - // The destination / target context. This should only be used when the source - // won't catch the error that results from the value it is passing over the - // bridge. This can **only** occur when returning a value from a function as - // we convert the return value after the method has terminated and execution - // has been returned to the caller. In this scenario the error will the be - // catchable in the "destination" context and therefore we create the error - // there. - kDestination -}; - } // namespace v8::MaybeLocal PassValueToOtherContext( @@ -153,7 +137,7 @@ v8::MaybeLocal PassValueToOtherContext( context_bridge::ObjectCache* object_cache, bool support_dynamic_properties, int recursion_depth, - BridgeErrorTarget error_target = BridgeErrorTarget::kSource) { + BridgeErrorTarget error_target) { TRACE_EVENT0("electron", "ContextBridge::PassValueToOtherContext"); if (recursion_depth >= kMaxRecursion) { v8::Context::Scope error_scope(error_target == BridgeErrorTarget::kSource @@ -289,6 +273,18 @@ v8::MaybeLocal PassValueToOtherContext( // re-construct in the destination context if (value->IsNativeError()) { v8::Context::Scope destination_context_scope(destination_context); + // We should try to pull "message" straight off of the error as a + // v8::Message includes some pretext that can get duplicated each time it + // crosses the bridge we fallback to the v8::Message approach if we can't + // pull "message" for some reason + v8::MaybeLocal maybe_message = value.As()->Get( + source_context, + gin::ConvertToV8(source_context->GetIsolate(), "message")); + v8::Local message; + if (maybe_message.ToLocal(&message) && message->IsString()) { + return v8::MaybeLocal( + v8::Exception::Error(message.As())); + } return v8::MaybeLocal(v8::Exception::Error( v8::Exception::CreateMessage(destination_context->GetIsolate(), value) ->Get())); diff --git a/shell/renderer/api/electron_api_context_bridge.h b/shell/renderer/api/electron_api_context_bridge.h index fdae88a06a..10392e5959 100644 --- a/shell/renderer/api/electron_api_context_bridge.h +++ b/shell/renderer/api/electron_api_context_bridge.h @@ -18,6 +18,31 @@ namespace api { void ProxyFunctionWrapper(const v8::FunctionCallbackInfo& info); +// Where the context bridge should create the exception it is about to throw +enum class BridgeErrorTarget { + // The source / calling context. This is default and correct 99% of the time, + // the caller / context asking for the conversion will receive the error and + // therefore the error should be made in that context + kSource, + // The destination / target context. This should only be used when the source + // won't catch the error that results from the value it is passing over the + // bridge. This can **only** occur when returning a value from a function as + // we convert the return value after the method has terminated and execution + // has been returned to the caller. In this scenario the error will the be + // catchable in the "destination" context and therefore we create the error + // there. + kDestination +}; + +v8::MaybeLocal PassValueToOtherContext( + v8::Local source_context, + v8::Local destination_context, + v8::Local value, + context_bridge::ObjectCache* object_cache, + bool support_dynamic_properties, + int recursion_depth, + BridgeErrorTarget error_target = BridgeErrorTarget::kSource); + v8::MaybeLocal CreateProxyForAPI( const v8::Local& api_object, const v8::Local& source_context, diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index 145dd0a018..20bb72dcbe 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -26,6 +26,8 @@ #include "shell/common/gin_helper/promise.h" #include "shell/common/node_includes.h" #include "shell/common/options_switches.h" +#include "shell/renderer/api/context_bridge/object_cache.h" +#include "shell/renderer/api/electron_api_context_bridge.h" #include "shell/renderer/api/electron_api_spell_check_client.h" #include "shell/renderer/electron_renderer_client.h" #include "third_party/blink/public/common/browser_interface_broker_proxy.h" @@ -160,21 +162,25 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { void CopyResultToCallingContextAndFinalize( v8::Isolate* isolate, - const v8::Local& result) { - blink::CloneableMessage ret; - bool success; - std::string error_message; + const v8::Local& result) { + v8::MaybeLocal maybe_result; + bool success = true; + std::string error_message = + "An unknown exception occurred while getting the result of the script"; { v8::TryCatch try_catch(isolate); - success = gin::ConvertFromV8(isolate, result, &ret); + context_bridge::ObjectCache object_cache; + maybe_result = PassValueToOtherContext(result->CreationContext(), + promise_.GetContext(), result, + &object_cache, false, 0); + if (maybe_result.IsEmpty() || try_catch.HasCaught()) { + success = false; + } 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 (!message.IsEmpty()) { + gin::ConvertFromV8(isolate, message->Get(), &error_message); } } } @@ -190,7 +196,7 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { } else { v8::Local context = promise_.GetContext(); v8::Context::Scope context_scope(context); - v8::Local cloned_value = gin::ConvertToV8(isolate, ret); + v8::Local cloned_value = maybe_result.ToLocalChecked(); if (callback_) std::move(callback_).Run(cloned_value, v8::Undefined(isolate)); promise_.Resolve(cloned_value); @@ -213,7 +219,8 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { value.As()->CreationContext()) && value->IsObject(); if (should_clone_value) { - CopyResultToCallingContextAndFinalize(isolate, value); + CopyResultToCallingContextAndFinalize(isolate, + value.As()); } else { // Right now only single results per frame is supported. if (callback_) diff --git a/spec-main/api-context-bridge-spec.ts b/spec-main/api-context-bridge-spec.ts index e65b649b3a..d5ed0622cc 100644 --- a/spec-main/api-context-bridge-spec.ts +++ b/spec-main/api-context-bridge-spec.ts @@ -283,7 +283,7 @@ describe('contextBridge', () => { return err; } }); - expect(result).to.be.an.instanceOf(Error).with.property('message', 'Uncaught Error: i-rejected'); + expect(result).to.be.an.instanceOf(Error).with.property('message', 'i-rejected'); }); it('should proxy nested promises and reject with the correct value', async () => { @@ -300,7 +300,7 @@ describe('contextBridge', () => { return err; } }); - expect(result).to.be.an.instanceOf(Error).with.property('message', 'Uncaught Error: i-rejected'); + expect(result).to.be.an.instanceOf(Error).with.property('message', 'i-rejected'); }); it('should proxy promises and resolve with the correct value if it resolves later', async () => { @@ -862,6 +862,12 @@ describe('contextBridge', () => { getArr: () => [123, 'string', true, ['foo']], getPromise: async () => ({ number: 123, string: 'string', boolean: true, fn: () => 'string', arr: [123, 'string', true, ['foo']] }), getFunctionFromFunction: async () => () => null, + getError: () => new Error('foo'), + getWeirdError: () => { + const e = new Error('foo'); + e.message = { garbage: true } as any; + return e; + }, object: { number: 123, string: 'string', @@ -921,6 +927,10 @@ describe('contextBridge', () => { [cleanedRoot.getFunctionFromFunction, Function], [cleanedRoot.getFunctionFromFunction(), Promise], [await cleanedRoot.getFunctionFromFunction(), Function], + [cleanedRoot.getError(), Error], + [cleanedRoot.getError().message, String], + [cleanedRoot.getWeirdError(), Error], + [cleanedRoot.getWeirdError().message, String], [cleanedRoot.getPromise(), Promise], [await cleanedRoot.getPromise(), Object], [(await cleanedRoot.getPromise()).number, Number],