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

Allow enabling <webview> tag with node integration disabled #9397

Merged
merged 18 commits into from May 19, 2017

Conversation

Projects
None yet
3 participants
@juturu
Copy link
Contributor

juturu commented May 7, 2017

No description provided.

juturu added some commits Nov 16, 2016

PR 44648: Enabling creation on webview with node-integration disabled…
… and raising events

 - Enabling creation on webview with node-integration disabled and raising events

Conflicts:
	lib/browser/guest-view-manager.js

@juturu juturu requested a review from kevinsawicki May 7, 2017

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented May 7, 2017

@juturu Can you explain the reasoning behind this, it seems very unsafe to me to have this option.

@juturu

This comment has been minimized.

Copy link
Contributor Author

juturu commented May 7, 2017

@MarshallOfSound it is to enable the scenarios explained in #2749 (comment) &
#2749 (comment)

Added events which can help clear the preload scripts in webview. By default electron having this security feature enabled. If developer is willing to override it and knows the risk then i feel adding this flexibility will make it easier for folks doing workarounds to achieve similar result.

@chandrachivukula @laramies to pitch in (if required).

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented May 7, 2017

@juturu I have concerns that this is making doing a bad thing far too easy.

If developer is willing to override it and knows the risk

These two are not necessarily mutually inclusive, lot's of people don't read the security guidelines for Electron (I see so many people loading remote content with nodeIntegration enabled for instance) and giving people a simple boolean option to do this makes it way to easy for them to expose their users to a multitude of exploits.

There is already a way to achieve what this option does but it requires a much greater understanding from the developer as to what they are actually doing.

  • Enable nodeIntegration
  • Delete all node globals in a preload script

Now you have the WebView element and no node environment, but it's a much more definitive as to what you are actually doing.

Interested in feedback from others RE the user safety issues raised above @electron/maintainers

@juturu

This comment has been minimized.

Copy link
Contributor Author

juturu commented May 8, 2017

@MarshallOfSound good point on the security concerns you raised. Here are couple of things i think we can do to make this flag more secure.

giving people a simple boolean option to do this makes it way to easy

Does making the boolean something like overrideWebViewSecurityFeature make your concern less worrysome?
Also, to reduce the security concern further, how about removing preload scripts from webView if nodeIntegration on the parent is turned off by electron.

@@ -218,6 +218,17 @@ When in-page navigation happens, the page URL changes but does not cause
navigation outside of the page. Examples of this occurring are when anchor links
are clicked or when the DOM `hashchange` event is triggered.

#### Event: 'will-create-webview'

This comment has been minimized.

@kevinsawicki

kevinsawicki May 8, 2017

Contributor

We already have will-attach-webview, could that be used instead?

https://electron.atom.io/docs/api/web-contents/#event-will-attach-webview

This comment has been minimized.

@juturu

juturu May 9, 2017

Author Contributor

Yes, that works!

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented May 8, 2017

Does making the boolean something like overrideWebViewSecurityFeature make your concern less worrysome?

That would make it a lot more clear yes 👍

Also, to reduce the security concern further, how about removing preload scripts from webView if nodeIntegration on the parent is turned off by electron.

Not sure what the use case is for a webview without nodeInegration in the parent but IMO if the parent doesn't have nodeIntegration then the webview shouldn't have a preload script OR the option to enable nodeIntegration. It should have no way to access node API's if the parent can't

juturu and others added some commits May 9, 2017

HariJ HariJ
@@ -406,6 +406,9 @@ void WebContents::InitWithSessionAndOptions(v8::Isolate* isolate,

Init(isolate);
AttachAsUserData(web_contents);

if (IsGuest())
Emit("did-create-webview");

This comment has been minimized.

@kevinsawicki

kevinsawicki May 9, 2017

Contributor

Do we still need this? We have web-contents-created that is emitted for each webContents and webContents.getType() to know which type was created.

* `overrideWebViewSecurity` Boolean (optional) - Whether to enable [webview-tag](webview-tag.md)
ignoring the security restriction based on `nodeIntegration`. Enabling this option will
have security implication on creating `webview` with `nodeIntegration` disabled. To avoid the
security risk, listen to `will-create-webview` event on [web-contents](web-contents.md) and

This comment has been minimized.

@kevinsawicki

kevinsawicki May 9, 2017

Contributor

will-create-webview is no longer here so this line needs to be updated.

@kevinsawicki kevinsawicki changed the title Add option to override webview creation with node integration disabled Allow enabling <webview> tag with node integration disabled May 17, 2017

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 17, 2017

Does making the boolean something like overrideWebViewSecurityFeature make your concern less worrysome?

I've updated this pull request to switch it to be called webviewTag that defaults to the value of the nodeIntegration option for now but we could make it default to false in Electron 2.0 so that apps always explicitly have to opt-in to the <webview> tag since it isn't a well-known concept/tag and would seem safer to have off by default eventually to prevent any unexpected behavior.

I've also made it so opened windows will have the <webview> tag disabled if it is disabled on the parent window to mirror the other inherited security options like nodeIntegration, contextIsolation, and javascript.

Also, to reduce the security concern further, how about removing preload scripts from webView if nodeIntegration on the parent is turned off by electron.

I've updated the security guide and other docs to mention how will-attach-webview should be used to validate things and strip preload scripts.

I'm open to going further and preventing them completely in this case but for remote content that runs a bit of local content, they seem easier to use than having the window do a webContents.executeJavaScript to inject things into the webview.

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 17, 2017

Does making the boolean something like overrideWebViewSecurityFeature make your concern less worrysome?

So if we go with an option that simply enables or disables it, like webviewTag, then apps that don't use the <webview> tag at all can set it to false and Electron will require two less files at startup and set less globals which seems nice.

@kevinsawicki kevinsawicki self-assigned this May 17, 2017

@kevinsawicki kevinsawicki merged commit 8404bdd into master May 19, 2017

4 of 9 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-linux-arm Build #6695863 queued
Details
electron-linux-ia32 Build #6695864 queued
Details
electron-linux-x64 Build #6695865 queued
Details
electron-mas-x64 Build #4259 succeeded in 10 min
Details
electron-osx-x64 Build #4263 succeeded in 9 min 39 sec
Details
electron-win-ia32 Build #3241 succeeded in 8 min 58 sec
Details
electron-win-x64 Build #3212 succeeded in 10 min
Details

@kevinsawicki kevinsawicki deleted the enable-webview branch May 19, 2017

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 19, 2017

Thanks for this @juturu 👍

@juturu

This comment has been minimized.

Copy link
Contributor Author

juturu commented May 19, 2017

Thanks @kevinsawicki 🙇

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