From 48a88790e7b38474d66706d3a51a3ed5eb4d1a3a Mon Sep 17 00:00:00 2001
From: Samuel Attard <samuel.r.attard@gmail.com>
Date: Mon, 13 Jul 2020 13:41:23 -0700
Subject: [PATCH] fix: ensure that errors thrown in the context bridge are
 created in the correct context

---
 .../api/electron_api_context_bridge.cc        | 38 +++++++++++++++----
 spec-main/api-context-bridge-spec.ts          | 33 ++++++++++++++++
 2 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/shell/renderer/api/electron_api_context_bridge.cc b/shell/renderer/api/electron_api_context_bridge.cc
index e2436171d69bc..e85c531d1af2c 100644
--- a/shell/renderer/api/electron_api_context_bridge.cc
+++ b/shell/renderer/api/electron_api_context_bridge.cc
@@ -127,6 +127,22 @@ 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 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(
@@ -135,10 +151,13 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
     v8::Local<v8::Value> value,
     context_bridge::ObjectCache* object_cache,
     bool support_dynamic_properties,
-    int recursion_depth) {
+    int recursion_depth,
+    BridgeErrorTarget error_target = BridgeErrorTarget::kSource) {
   TRACE_EVENT0("electron", "ContextBridge::PassValueToOtherContext");
   if (recursion_depth >= kMaxRecursion) {
-    v8::Context::Scope source_scope(source_context);
+    v8::Context::Scope error_scope(error_target == BridgeErrorTarget::kSource
+                                       ? source_context
+                                       : destination_context);
     source_context->GetIsolate()->ThrowException(v8::Exception::TypeError(
         gin::StringToV8(source_context->GetIsolate(),
                         "Electron contextBridge recursion depth exceeded.  "
@@ -314,9 +333,12 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
   // Serializable objects
   blink::CloneableMessage ret;
   {
-    v8::Context::Scope source_context_scope(source_context);
+    v8::Local<v8::Context> error_context =
+        error_target == BridgeErrorTarget::kSource ? source_context
+                                                   : destination_context;
+    v8::Context::Scope error_scope(error_context);
     // V8 serializer will throw an error if required
-    if (!gin::ConvertFromV8(source_context->GetIsolate(), value, &ret))
+    if (!gin::ConvertFromV8(error_context->GetIsolate(), value, &ret))
       return v8::MaybeLocal<v8::Value>();
   }
 
@@ -402,10 +424,10 @@ void ProxyFunctionWrapper(const v8::FunctionCallbackInfo<v8::Value>& info) {
     if (maybe_return_value.IsEmpty())
       return;
 
-    auto ret =
-        PassValueToOtherContext(func_owning_context, calling_context,
-                                maybe_return_value.ToLocalChecked(),
-                                &object_cache, support_dynamic_properties, 0);
+    auto ret = PassValueToOtherContext(
+        func_owning_context, calling_context,
+        maybe_return_value.ToLocalChecked(), &object_cache,
+        support_dynamic_properties, 0, BridgeErrorTarget::kDestination);
     if (ret.IsEmpty())
       return;
     info.GetReturnValue().Set(ret.ToLocalChecked());
diff --git a/spec-main/api-context-bridge-spec.ts b/spec-main/api-context-bridge-spec.ts
index 1e983539987ff..b74ca45a09a94 100644
--- a/spec-main/api-context-bridge-spec.ts
+++ b/spec-main/api-context-bridge-spec.ts
@@ -486,6 +486,39 @@ describe('contextBridge', () => {
         expect(threw).to.equal(true);
       });
 
+      it('should copy thrown errors into the other context', async () => {
+        await makeBindingWindow(() => {
+          contextBridge.exposeInMainWorld('example', {
+            throwNormal: () => {
+              throw new Error('whoops');
+            },
+            throwWeird: () => {
+              throw 'this is no error...'; // eslint-disable-line no-throw-literal
+            },
+            throwNotClonable: () => {
+              return Object(Symbol('foo'));
+            },
+            argumentConvert: () => {}
+          });
+        });
+        const result = await callWithBindings((root: any) => {
+          const getError = (fn: Function) => {
+            try {
+              fn();
+            } catch (e) {
+              return e;
+            }
+            return null;
+          };
+          const normalIsError = Object.getPrototypeOf(getError(root.example.throwNormal)) === Error.prototype;
+          const weirdIsError = Object.getPrototypeOf(getError(root.example.throwWeird)) === Error.prototype;
+          const notClonableIsError = Object.getPrototypeOf(getError(root.example.throwNotClonable)) === Error.prototype;
+          const argumentConvertIsError = Object.getPrototypeOf(getError(() => root.example.argumentConvert(Object(Symbol('test'))))) === Error.prototype;
+          return [normalIsError, weirdIsError, notClonableIsError, argumentConvertIsError];
+        });
+        expect(result).to.deep.equal([true, true, true, true], 'should all be errors in the current context');
+      });
+
       it('should not leak prototypes', async () => {
         await makeBindingWindow(() => {
           contextBridge.exposeInMainWorld('example', {