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

@@ -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

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))

shell/renderer/api/electron_api_context_bridge.cc

@@ -128,22 +128,6 @@ v8::MaybeLocal<v8::Value> GetPrivate(v8::Local<v8::Context> 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<v8::Value> PassValueToOtherContext(
@@ -153,7 +137,7 @@ v8::MaybeLocal<v8::Value> 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<v8::Value> 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<v8::Value> maybe_message = value.As<v8::Object>()->Get(
+        source_context,
+        gin::ConvertToV8(source_context->GetIsolate(), "message"));
+    v8::Local<v8::Value> message;
+    if (maybe_message.ToLocal(&message) && message->IsString()) {
+      return v8::MaybeLocal<v8::Value>(
+          v8::Exception::Error(message.As<v8::String>()));
+    }
     return v8::MaybeLocal<v8::Value>(v8::Exception::Error(
         v8::Exception::CreateMessage(destination_context->GetIsolate(), value)
             ->Get()));

shell/renderer/api/electron_api_context_bridge.h

@@ -18,6 +18,31 @@ namespace api {
 
 void ProxyFunctionWrapper(const v8::FunctionCallbackInfo<v8::Value>& 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<v8::Value> PassValueToOtherContext(
+    v8::Local<v8::Context> source_context,
+    v8::Local<v8::Context> destination_context,
+    v8::Local<v8::Value> value,
+    context_bridge::ObjectCache* object_cache,
+    bool support_dynamic_properties,
+    int recursion_depth,
+    BridgeErrorTarget error_target = BridgeErrorTarget::kSource);
+
 v8::MaybeLocal<v8::Object> CreateProxyForAPI(
     const v8::Local<v8::Object>& api_object,
     const v8::Local<v8::Context>& source_context,

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<v8::Value>& result) {
+      const v8::Local<v8::Object>& result) {
-    blink::CloneableMessage ret;
+    v8::MaybeLocal<v8::Value> maybe_result;
-    bool success;
+    bool success = true;
-    std::string error_message;
+    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() ||
+        if (!message.IsEmpty()) {
-            !gin::ConvertFromV8(isolate, message->Get(), &error_message)) {
+          gin::ConvertFromV8(isolate, message->Get(), &error_message);
-          error_message =
-              "An unknown exception occurred while getting the result of "
-              "the script";
         }
       }
     }
@@ -190,7 +196,7 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback {
     } else {
       v8::Local<v8::Context> context = promise_.GetContext();
       v8::Context::Scope context_scope(context);
-      v8::Local<v8::Value> cloned_value = gin::ConvertToV8(isolate, ret);
+      v8::Local<v8::Value> 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<v8::Object>()->CreationContext()) &&
             value->IsObject();
         if (should_clone_value) {
-          CopyResultToCallingContextAndFinalize(isolate, value);
+          CopyResultToCallingContextAndFinalize(isolate,
+                                                value.As<v8::Object>());
         } else {
           // Right now only single results per frame is supported.
           if (callback_)

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],