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

fix: illegal access errors with nodeIntegrationInSubFrames #29170

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 15 additions & 2 deletions shell/renderer/electron_render_frame_observer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ void ElectronRenderFrameObserver::DidInstallConditionalFeatures(
bool is_not_opened = !render_frame_->GetWebFrame()->Opener() ||
prefs.node_leakage_in_renderers;
bool allow_node_in_sub_frames = prefs.node_integration_in_sub_frames;

bool should_create_isolated_context =
use_context_isolation && is_main_world &&
(is_main_frame || allow_node_in_sub_frames) &&
Expand Down Expand Up @@ -155,12 +156,24 @@ bool ElectronRenderFrameObserver::IsIsolatedWorld(int world_id) {

bool ElectronRenderFrameObserver::ShouldNotifyClient(int world_id) {
auto prefs = render_frame_->GetBlinkPreferences();

// This is necessary because if an iframe is created and a source is not
// set, the iframe loads about:blank and creates a script context for the
// same. We don't want to create a Node.js environment here because if the src
// is later set, the JS necessary to do that triggers illegal access errors
// when the initial about:blank Node.js environment is cleaned up. See:
// https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/render_frame_impl.h;l=870-892;drc=4b6001440a18740b76a1c63fa2a002cc941db394
GURL url = render_frame_->GetWebFrame()->GetDocument().Url();
bool allow_node_in_sub_frames = prefs.node_integration_in_sub_frames;
if (allow_node_in_sub_frames && url.IsAboutBlank() &&
!render_frame_->IsMainFrame())
return false;

if (prefs.context_isolation &&
(render_frame_->IsMainFrame() || allow_node_in_sub_frames))
return IsIsolatedWorld(world_id);
else
return IsMainWorld(world_id);

return IsMainWorld(world_id);
}

} // namespace electron
5 changes: 3 additions & 2 deletions shell/renderer/electron_renderer_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ void ElectronRendererClient::DidCreateScriptContext(
// TODO(zcbenz): Do not create Node environment if node integration is not
// enabled.

// Only load node if we are a main frame or a devtools extension
// unless node support has been explicitly enabled for sub frames
// Only load Node.js if we are a main frame or a devtools extension
// unless Node.js support has been explicitly enabled for subframes.
auto prefs = render_frame->GetBlinkPreferences();
bool reuse_renderer_processes_enabled =
prefs.disable_electron_site_instance_overrides;
Expand All @@ -97,6 +97,7 @@ void ElectronRendererClient::DidCreateScriptContext(
(is_not_opened || reuse_renderer_processes_enabled);
bool is_devtools = IsDevToolsExtension(render_frame);
bool allow_node_in_subframes = prefs.node_integration_in_sub_frames;

bool should_load_node =
(is_main_frame || is_devtools || allow_node_in_subframes) &&
!IsWebViewFrame(renderer_context, render_frame);
Expand Down
2 changes: 2 additions & 0 deletions shell/renderer/electron_sandboxed_renderer_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,10 @@ void ElectronSandboxedRendererClient::DidCreateScriptContext(
bool is_main_frame = render_frame->IsMainFrame();
bool is_devtools =
IsDevTools(render_frame) || IsDevToolsExtension(render_frame);

bool allow_node_in_sub_frames =
render_frame->GetBlinkPreferences().node_integration_in_sub_frames;

bool should_load_preload =
(is_main_frame || is_devtools || allow_node_in_sub_frames) &&
!IsWebViewFrame(context, render_frame);
Expand Down
29 changes: 29 additions & 0 deletions spec-main/fixtures/crash-cases/js-execute-iframe/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<html>
<body>
<iframe id="mainframe"></iframe>
<script>
const net = require('net');
const path = require('path');

document.getElementById("mainframe").src="./page2.html";

const p = process.platform === 'win32'
? path.join('\\\\?\\pipe', process.cwd(), 'myctl')
: '/tmp/echo.sock';

const client = net.createConnection({ path: p }, () => {
console.log('connected to server');
client.write('world!\r\n');
});

client.on('data', (data) => {
console.log(data.toString());
client.end();
});

client.on('end', () => {
console.log('disconnected from server');
});
</script>
</body>
</html>
51 changes: 51 additions & 0 deletions spec-main/fixtures/crash-cases/js-execute-iframe/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
const { app, BrowserWindow } = require('electron');
const net = require('net');
const path = require('path');

function createWindow () {
const mainWindow = new BrowserWindow({
webPreferences: {
nodeIntegration: true,
contextIsolation: false,
nodeIntegrationInSubFrames: true
}
});

mainWindow.loadFile('index.html');
}

app.whenReady().then(() => {
createWindow();

app.on('activate', () => {
if (BrowserWindow.getAllWindows().length === 0) createWindow();
});
});

app.on('window-all-closed', () => {
if (process.platform !== 'darwin') app.quit();
});

const server = net.createServer((c) => {
console.log('client connected');

c.on('end', () => {
console.log('client disconnected');
app.quit();
});

c.write('hello\r\n');
c.pipe(c);
});

server.on('error', (err) => {
throw err;
});

const p = process.platform === 'win32'
? path.join('\\\\?\\pipe', process.cwd(), 'myctl')
: '/tmp/echo.sock';

server.listen(p, () => {
console.log('server bound');
});
4 changes: 4 additions & 0 deletions spec-main/fixtures/crash-cases/js-execute-iframe/page2.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<!DOCTYPE html>
<html>
<body>HELLO</body>
</html>