Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sub frames should get an isolated context when node is enabled in sub frames #16804

Closed
samuelmaddock opened this issue Feb 7, 2019 · 0 comments · Fixed by #17165
Closed

Sub frames should get an isolated context when node is enabled in sub frames #16804

samuelmaddock opened this issue Feb 7, 2019 · 0 comments · Fixed by #17165
Assignees
Projects

Comments

@samuelmaddock
Copy link
Member

samuelmaddock commented Feb 7, 2019

  • Electron Version (output of node_modules/.bin/electron --version):
    • 5.x.x

Expected Behavior

Context isolation should be enabled in subframes when the nodeIntegrationInSubFrames option is provided.

Actual behavior

Context isolation is not enabled for node environments in subframes.

To Reproduce

Enable nodeIntegrationInSubFrames and load any content in a subframe.

Additional Information

See #16425 and https://github.com/electron/electron/blob/master/atom/renderer/atom_render_frame_observer.cc#L92-L104

I started working on a fix, but wasn't sure if the preload_bundle was actually being run in the correct context.

diff --git a/atom/renderer/atom_render_frame_observer.cc b/atom/renderer/atom_render_frame_observer.cc
index f8a54a199..b878954cc 100644
--- a/atom/renderer/atom_render_frame_observer.cc
+++ b/atom/renderer/atom_render_frame_observer.cc
@@ -12,6 +12,8 @@
 #include "atom/common/heap_snapshot.h"
 #include "atom/common/native_mate_converters/value_converter.h"
 #include "atom/common/node_includes.h"
+#include "atom/common/options_switches.h"
+#include "base/command_line.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/trace_event/trace_event.h"
@@ -95,9 +97,17 @@ void AtomRenderFrameObserver::DidCreateScriptContext(
   if (ShouldNotifyClient(world_id))
     renderer_client_->DidCreateScriptContext(context, render_frame_);
 
-  if (renderer_client_->isolated_world() && IsMainWorld(world_id) &&
-      // Only the top window's main frame has isolated world.
-      render_frame_->IsMainFrame() && !render_frame_->GetWebFrame()->Opener()) {
+  bool use_context_isolation = renderer_client_->isolated_world();
+  bool is_main_world = IsMainWorld(world_id);
+  bool is_main_frame = render_frame_->IsMainFrame();
+  bool is_not_opened = !render_frame_->GetWebFrame()->Opener();
+  bool allow_node_in_sub_frames =
+      base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kNodeIntegrationInSubFrames);
+  bool should_create_isolated_context =
+      use_context_isolation && (is_main_frame || allow_node_in_sub_frames) && is_main_world && is_not_opened;
+
+  if (should_create_isolated_context) {
     CreateIsolatedWorldContext();
     renderer_client_->SetupMainWorldOverrides(context, render_frame_);
   }
@sofianguy sofianguy added this to Unsorted Issues in 5.0.x Feb 7, 2019
@sofianguy sofianguy moved this from Unsorted Issues to Doesn't Block Stable in 5.0.x Feb 14, 2019
@samuelmaddock samuelmaddock self-assigned this Feb 27, 2019
@sofianguy sofianguy moved this from Doesn't Block Stable to Fixed for Next Release in 5.0.x Mar 20, 2019
@sofianguy sofianguy moved this from Fixed for Next Release to Fixed in 5.0.0-beta.6 in 5.0.x Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
5.0.x
Fixed in 5.0.0-beta.6
Development

Successfully merging a pull request may close this issue.

2 participants