Skip to content

Commit

Permalink
fix: exceptions in nested conversions live in the target world (#37897)
Browse files Browse the repository at this point in the history 
Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Samuel Attard <marshallofsound@electronjs.org>
  • Loading branch information
@trop @MarshallOfSound
trop[bot] and MarshallOfSound committed Apr 11, 2023
1 parent 9f1bb29 commit 4de542d
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 30 deletions.
52 changes: 28 additions & 24 deletions shell/renderer/api/electron_api_context_bridge.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,10 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
global_destination_context.IsEmpty())
return;
context_bridge::ObjectCache object_cache;
auto val =
PassValueToOtherContext(global_source_context.Get(isolate),
global_destination_context.Get(isolate),
result, &object_cache, false, 0);
auto val = PassValueToOtherContext(
global_source_context.Get(isolate),
global_destination_context.Get(isolate), result, &object_cache,
false, 0, BridgeErrorTarget::kSource);
if (!val.IsEmpty())
proxied_promise->Resolve(val.ToLocalChecked());
},
Expand All @@ -268,10 +268,10 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
global_destination_context.IsEmpty())
return;
context_bridge::ObjectCache object_cache;
auto val =
PassValueToOtherContext(global_source_context.Get(isolate),
global_destination_context.Get(isolate),
result, &object_cache, false, 0);
auto val = PassValueToOtherContext(
global_source_context.Get(isolate),
global_destination_context.Get(isolate), result, &object_cache,
false, 0, BridgeErrorTarget::kSource);
if (!val.IsEmpty())
proxied_promise->Reject(val.ToLocalChecked());
},
Expand Down Expand Up @@ -324,7 +324,7 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
auto value_for_array = PassValueToOtherContext(
source_context, destination_context,
arr->Get(source_context, i).ToLocalChecked(), object_cache,
support_dynamic_properties, recursion_depth + 1);
support_dynamic_properties, recursion_depth + 1, error_target);
if (value_for_array.IsEmpty())
return v8::MaybeLocal<v8::Value>();

Expand Down Expand Up @@ -358,7 +358,7 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
auto object_value = value.As<v8::Object>();
auto passed_value = CreateProxyForAPI(
object_value, source_context, destination_context, object_cache,
support_dynamic_properties, recursion_depth + 1);
support_dynamic_properties, recursion_depth + 1, error_target);
if (passed_value.IsEmpty())
return v8::MaybeLocal<v8::Value>();
return v8::MaybeLocal<v8::Value>(passed_value.ToLocalChecked());
Expand All @@ -372,8 +372,9 @@ v8::MaybeLocal<v8::Value> PassValueToOtherContext(
: destination_context;
v8::Context::Scope error_scope(error_context);
// V8 serializer will throw an error if required
if (!gin::ConvertFromV8(error_context->GetIsolate(), value, &ret))
if (!gin::ConvertFromV8(error_context->GetIsolate(), value, &ret)) {
return v8::MaybeLocal<v8::Value>();
}
}

{
Expand Down Expand Up @@ -420,9 +421,9 @@ void ProxyFunctionWrapper(const v8::FunctionCallbackInfo<v8::Value>& info) {
args.GetRemaining(&original_args);

for (auto value : original_args) {
auto arg =
PassValueToOtherContext(calling_context, func_owning_context, value,
&object_cache, support_dynamic_properties, 0);
auto arg = PassValueToOtherContext(
calling_context, func_owning_context, value, &object_cache,
support_dynamic_properties, 0, BridgeErrorTarget::kSource);
if (arg.IsEmpty())
return;
proxied_args.push_back(arg.ToLocalChecked());
Expand Down Expand Up @@ -485,7 +486,8 @@ v8::MaybeLocal<v8::Object> CreateProxyForAPI(
const v8::Local<v8::Context>& destination_context,
context_bridge::ObjectCache* object_cache,
bool support_dynamic_properties,
int recursion_depth) {
int recursion_depth,
BridgeErrorTarget error_target) {
gin_helper::Dictionary api(source_context->GetIsolate(), api_object);

{
Expand Down Expand Up @@ -526,14 +528,16 @@ v8::MaybeLocal<v8::Object> CreateProxyForAPI(
if (!getter.IsEmpty()) {
if (!PassValueToOtherContext(source_context, destination_context,
getter, object_cache,
support_dynamic_properties, 1)
support_dynamic_properties, 1,
error_target)
.ToLocal(&getter_proxy))
continue;
}
if (!setter.IsEmpty()) {
if (!PassValueToOtherContext(source_context, destination_context,
setter, object_cache,
support_dynamic_properties, 1)
support_dynamic_properties, 1,
error_target)
.ToLocal(&setter_proxy))
continue;
}
Expand All @@ -551,7 +555,7 @@ v8::MaybeLocal<v8::Object> CreateProxyForAPI(

auto passed_value = PassValueToOtherContext(
source_context, destination_context, value, object_cache,
support_dynamic_properties, recursion_depth + 1);
support_dynamic_properties, recursion_depth + 1, error_target);
if (passed_value.IsEmpty())
return v8::MaybeLocal<v8::Object>();
proxy.Set(key, passed_value.ToLocalChecked());
Expand Down Expand Up @@ -597,9 +601,9 @@ void ExposeAPIInWorld(v8::Isolate* isolate,
context_bridge::ObjectCache object_cache;
v8::Context::Scope target_context_scope(target_context);

v8::MaybeLocal<v8::Value> maybe_proxy =
PassValueToOtherContext(electron_isolated_context, target_context, api,
&object_cache, false, 0);
v8::MaybeLocal<v8::Value> maybe_proxy = PassValueToOtherContext(
electron_isolated_context, target_context, api, &object_cache, false, 0,
BridgeErrorTarget::kSource);
if (maybe_proxy.IsEmpty())
return;
auto proxy = maybe_proxy.ToLocalChecked();
Expand Down Expand Up @@ -649,7 +653,7 @@ void OverrideGlobalValueFromIsolatedWorld(
context_bridge::ObjectCache object_cache;
v8::MaybeLocal<v8::Value> maybe_proxy = PassValueToOtherContext(
value->GetCreationContextChecked(), main_context, value, &object_cache,
support_dynamic_properties, 1);
support_dynamic_properties, 1, BridgeErrorTarget::kSource);
DCHECK(!maybe_proxy.IsEmpty());
auto proxy = maybe_proxy.ToLocalChecked();

Expand Down Expand Up @@ -685,14 +689,14 @@ bool OverrideGlobalPropertyFromIsolatedWorld(
if (!getter->IsNullOrUndefined()) {
v8::MaybeLocal<v8::Value> maybe_getter_proxy = PassValueToOtherContext(
getter->GetCreationContextChecked(), main_context, getter,
&object_cache, false, 1);
&object_cache, false, 1, BridgeErrorTarget::kSource);
DCHECK(!maybe_getter_proxy.IsEmpty());
getter_proxy = maybe_getter_proxy.ToLocalChecked();
}
if (!setter->IsNullOrUndefined() && setter->IsObject()) {
v8::MaybeLocal<v8::Value> maybe_setter_proxy = PassValueToOtherContext(
getter->GetCreationContextChecked(), main_context, setter,
&object_cache, false, 1);
&object_cache, false, 1, BridgeErrorTarget::kSource);
DCHECK(!maybe_setter_proxy.IsEmpty());
setter_proxy = maybe_setter_proxy.ToLocalChecked();
}
Expand Down
5 changes: 3 additions & 2 deletions shell/renderer/api/electron_api_context_bridge.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,16 @@ 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);

v8::MaybeLocal<v8::Object> CreateProxyForAPI(
const v8::Local<v8::Object>& api_object,
const v8::Local<v8::Context>& source_context,
const v8::Local<v8::Context>& destination_context,
context_bridge::ObjectCache* object_cache,
bool support_dynamic_properties,
int recursion_depth);
int recursion_depth,
BridgeErrorTarget error_target);

} // namespace electron::api

Expand Down
2 changes: 1 addition & 1 deletion shell/renderer/api/electron_api_web_frame.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class ScriptExecutionCallback {
context_bridge::ObjectCache object_cache;
maybe_result = PassValueToOtherContext(
result->GetCreationContextChecked(), promise_.GetContext(), result,
&object_cache, false, 0);
&object_cache, false, 0, BridgeErrorTarget::kSource);
if (maybe_result.IsEmpty() || try_catch.HasCaught()) {
success = false;
}
Expand Down
3 changes: 2 additions & 1 deletion shell/renderer/renderer_client_base.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,8 @@ void RendererClientBase::SetupMainWorldOverrides(
if (global.GetHidden("guestViewInternal", &guest_view_internal)) {
api::context_bridge::ObjectCache object_cache;
auto result = api::PassValueToOtherContext(
source_context, context, guest_view_internal, &object_cache, false, 0);
source_context, context, guest_view_internal, &object_cache, false, 0,
api::BridgeErrorTarget::kSource);
if (!result.IsEmpty()) {
isolated_api.Set("guestViewInternal", result.ToLocalChecked());
}
Expand Down
14 changes: 12 additions & 2 deletions spec/api-context-bridge-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,14 @@ describe('contextBridge', () => {
throwNotClonable: () => {
return Object(Symbol('foo'));
},
throwNotClonableNestedArray: () => {
return [Object(Symbol('foo'))];
},
throwNotClonableNestedObject: () => {
return {
bad: Object(Symbol('foo'))
};
},
argumentConvert: () => {}
});
});
Expand All @@ -817,10 +825,12 @@ describe('contextBridge', () => {
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 notClonableNestedArrayIsError = Object.getPrototypeOf(getError(root.example.throwNotClonableNestedArray)) === Error.prototype;
const notClonableNestedObjectIsError = Object.getPrototypeOf(getError(root.example.throwNotClonableNestedObject)) === Error.prototype;
const argumentConvertIsError = Object.getPrototypeOf(getError(() => root.example.argumentConvert(Object(Symbol('test'))))) === Error.prototype;
return [normalIsError, weirdIsError, notClonableIsError, argumentConvertIsError];
return [normalIsError, weirdIsError, notClonableIsError, notClonableNestedArrayIsError, notClonableNestedObjectIsError, argumentConvertIsError];
});
expect(result).to.deep.equal([true, true, true, true], 'should all be errors in the current context');
expect(result).to.deep.equal([true, true, true, true, true, true], 'should all be errors in the current context');
});

it('should not leak prototypes', async () => {
Expand Down

0 comments on commit 4de542d

Please sign in to comment.