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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable node on child window when disabled on parent #4897

Merged
merged 17 commits into from Apr 2, 2016

Conversation

Projects
None yet
5 participants
@kevinsawicki
Copy link
Contributor

kevinsawicki commented Mar 24, 2016

If nodeIntegration is disabled on the parent window then disable it on all child windows opened via window.open or in <webview> tags.

This prevents windows without node integration from opening new windows with node integration enabled.

This seems to be an intuitive behavior for this option, if you disable node on a window/webview then all downstream windows will also have node integration disabled preventing content run in those windows from "breaking out" and getting access to node integration via calls to window.open.

Fixes #3943
Fixes #4026

/cc @zeke 馃崘

it('disables node integration when it is disabled on the parent window', function(done) {
var b;
listener = function(event) {
assert.equal(event.data, 'undefined');

This comment has been minimized.

@zeke

zeke Mar 24, 2016

Member

It would be nice to come up with an assertion that's less abstact. I know when we paired we tested the failure case before adding the implementation, but on its own this code gives me very little confidence that the feature actually works.

This comment has been minimized.

@kevinsawicki

kevinsawicki Mar 24, 2016

Contributor

馃憤 I updated the assert to be for a isProcessGlobalUndefined value that is true.

This comment has been minimized.

@zeke

zeke Mar 24, 2016

Member

Nice.

@zeke

This comment has been minimized.

Copy link
Member

zeke commented Mar 24, 2016

Are the builds flaky?

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented Mar 24, 2016

Are the builds flaky?

Looks like the output was:

[TypeError: Cannot read property 'nodeIntegration' of undefined] 'TypeError: Cannot read property \'nodeIntegration\' of undefined
    at mergeBrowserWindowOptions (/var/lib/jenkins/workspace/electron-linux-x64/out/D/resources/atom.asar/browser/guest-window-manager.js:33:54)
    at EventEmitter.<anonymous> (/var/lib/jenkins/workspace/electron-linux-x64/out/D/resources/atom.asar/browser/guest-window-manager.js:89:13)
    at emitMany (events.js:108:13)
    at EventEmitter.emit (events.js:182:7)
    at EventEmitter.<anonymous> (/var/lib/jenkins/workspace/electron-linux-x64/out/D/resources/atom.asar/browser/api/web-contents.js:136:25)
    at emitTwo (events.js:87:13)
    at EventEmitter.emit (events.js:172:7)

So it looks like there may a case where a browserWindowOptions is missing webPreferences and so this new code is causing a flaky failure.

I'll investigate 馃攳

@kevinsawicki kevinsawicki force-pushed the node-integration-inheritance branch from 595d8ab to 027baad Mar 30, 2016

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented Mar 30, 2016

/cc @wearhere @lukeapage wanted to get your thoughts on this new behavior since you both commented on #3943 and #4026

@kevinsawicki kevinsawicki force-pushed the node-integration-inheritance branch from 8e61430 to 230ed78 Apr 1, 2016

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented Apr 2, 2016

@zcbenz any concerns with merging this? 馃憖

@zcbenz

This comment has been minimized.

Copy link
Contributor

zcbenz commented Apr 2, 2016

馃憤

@zcbenz zcbenz merged commit 7bfe80f into master Apr 2, 2016

7 of 8 checks passed

electron-win-ia32 Build #3004723 failed in 1800s
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #3004720 succeeded in 77s
Details
electron-linux-ia32 Build #3004721 succeeded in 82s
Details
electron-linux-x64 Build #3004722 succeeded in 101s
Details
electron-mas-x64 Build #747 succeeded in 4 min 44 sec
Details
electron-osx-x64 Build #769 succeeded in 5 min 19 sec
Details
electron-win-x64 Build #3004724 succeeded in 114s
Details

@zcbenz zcbenz deleted the node-integration-inheritance branch Apr 2, 2016

@laramies

This comment has been minimized.

Copy link

laramies commented Apr 14, 2016

@kevinsawicki: I am using Electron 0.37.5 and webviews still can enable nodeintegration even if the parent has nodeintegration disabled. I was expecting this PR to prevent this, right?

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented Apr 14, 2016

I am using Electron 0.37.5 and webviews still can enable nodeintegration even if the parent has nodeintegration disabled. I was expecting this PR to prevent this, right?

@laramies can you create a new issue with example code for this? Thanks

@laramies

This comment has been minimized.

Copy link

laramies commented Apr 21, 2016

@kevinsawicki I created #5171 but it was closed by @zcbenz as duplicate. Can you please confirm that it's going to be fixed, please?

@TimNZ

This comment has been minimized.

Copy link

TimNZ commented May 27, 2016

Does this PR address current security concerns about arbitrary script in remote pages accessing node resources? e.g. can I feel safe now that except for 0 day exploit that webview content cannot doing anything it couldn't do before if nodeintegration is not enabled?

In my quick testing it appears so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment