feat: add worldSafe flag for executeJS results #24114

MarshallOfSound · 2020-06-12T22:54:38Z

Previously the return values of webFrame.executeJavaScript crossed the world boundary when context isolation was enabled. This allows apps to makes themselves insecure by accidentally sending objects from the isolated world back to the main world. To help devs avoid this we're adding this new flag, and this flag will be turned on by default in Electron 12 (and removed) ensuring that this kind of issue can't become a thing again.

This PR is also requesting new minors of 8 and 9 😄

Notes: Added new worldSafeExecuteJavaScript webPreference to ensure that the return values from webFrame.executeJavaScript are world safe when context isolation is enabled

docs/api/browser-window.md

lib/renderer/api/web-frame.ts

shell/renderer/api/electron_api_web_frame.cc

spec-main/api-web-frame-spec.ts

shell/renderer/api/electron_api_web_frame.cc

Co-authored-by: Jeremy Rose <jeremya@chromium.org>

shell/renderer/api/electron_api_web_frame.cc

lib/renderer/api/web-frame.ts

lib/renderer/web-frame-init.ts

shell/renderer/api/electron_api_web_frame.cc

nornagon · 2020-07-20T22:30:17Z

docs/api/browser-window.md

+    * `worldSafeExecuteJavaScript` Boolean (optional) - If true, values returned from `webFrame.executeJavaScript` will be sanitized to ensure JS values
+      can't unsafely cross between worlds when using `contextIsolation`.  The default
+      is `false`. In Electron 12, the default will be changed to `true`. _Deprecated_


I wonder if this might be a good test case for the base::Feature-based stuff we've been talking about in api-wg?

Maybe, but I don't super want to figure out that system / pioneer it in this PR

Someone's gotta be the first :\

release-clerk · 2020-07-23T21:32:23Z

Release Notes Persisted

Added new worldSafeExecuteJavaScript webPreference to ensure that the return values from webFrame.executeJavaScript are world safe when context isolation is enabled

trop · 2020-07-23T21:32:40Z

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

trop · 2020-07-23T21:32:57Z

I have automatically backported this PR to "10-x-y", please check out #24711

trop · 2020-07-23T21:33:00Z

I have automatically backported this PR to "9-x-y", please check out #24712

<electron/electron#24114> "Previously the return values of webFrame.executeJavaScript crossed the world boundary when context isolation was enabled. This allows apps to makes themselves insecure by accidentally sending objects from the isolated world back to the main world. To help devs avoid this we're adding this new flag, and this flag will be turned on by default in Electron 12 (and removed) ensuring that this kind of issue can't become a thing again. This PR is also requesting new minors of 8 and 9 😄 Notes: Added new worldSafeExecuteJavaScript webPreference to ensure that the return values from webFrame.executeJavaScript are world safe when context isolation is enabled"

bughit · 2020-10-01T23:30:33Z

@MarshallOfSound

Please don't remove the option. I have code the defines (and returns) anon functions in the main world. That's not even hypothetically unsafe. Also code that returns other main world objects (e.g. window). The 'window' property is unwritable and unconfigurable, so that's also not unsafe, you're always get the expected window even in untrusted content. It's reasonable for this raw access be restricted by default, but the EIC should retain it optionally, it can be safe, especially if the content is trusted.

If contextIsolation can be disabled and nodeIntegration enabled, security is clearly ultimately up to the dev, it can't be forced like this.

bughit · 2020-10-05T23:14:23Z

@nornagon

Could we please have some discussion about the planned removal of worldSafeExecuteJavaScript. I have code that depends on it. Retrieves the main world window. Defines anon functions for later execution.

const contentWin = webFrame.executeJavaScript('window')
const contentFn = webFrame.executeJavaScript('() => {...}')

I have no issue with maximally secure defaults, but there shouldn't be a need to completely block this, breaking working code, forcing redesigns. . This can be used safely, judiciously. Considering security can be disabled completely, it's not unreasonable to optionally allow EIC to retrieve content world objects. Please don't remove the choice.

poiru · 2020-10-23T17:42:01Z

docs/api/browser-window.md

@@ -348,6 +348,9 @@ It creates a new `BrowserWindow` with native properties as set by the `options`.
      You can access this context in the dev tools by selecting the
      'Electron Isolated Context' entry in the combo box at the top of the
      Console tab.
+    * `worldSafeExecuteJavaScript` Boolean (optional) - If true, values returned from `webFrame.executeJavaScript` will be sanitized to ensure JS values
+      can't unsafely cross between worlds when using `contextIsolation`.  The default
+      is `false`. In Electron 12, the default will be changed to `true`. _Deprecated_


@MarshallOfSound Why is this marked Deprecated? I turned this on to prepare for Electron 12, but my code reviewer was confused why I'm using a deprecated option. Presumably false is deprecated, but I think we should be explicit about it.

@poiru This option is a migration feature, it's kinda weird and you're right we should probably document it better. The flow here will be

Add this option

Change default of this option

Deprecate this option in code

Remove this option

We aren't going to leave this flag around forever / for long at all

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 12, 2020

MarshallOfSound added target/10-x-y labels Jun 12, 2020

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 13, 2020

nornagon requested changes Jun 15, 2020

View reviewed changes

MarshallOfSound force-pushed the world-safe-execute-js branch from 49f8c7c to 3e2502a Compare June 24, 2020 16:50

MarshallOfSound requested a review from nornagon June 24, 2020 16:50

nornagon reviewed Jun 24, 2020

View reviewed changes

shell/renderer/api/electron_api_web_frame.cc Outdated Show resolved Hide resolved

nornagon reviewed Jun 24, 2020

View reviewed changes

shell/renderer/api/electron_api_web_frame.cc Outdated Show resolved Hide resolved

MarshallOfSound and others added 6 commits July 13, 2020 11:38

feat: add worldSafe flag for executeJS results

6e49e97

chore: do not log warning for webContents.executeJS

7d92ae5

Apply suggestions from code review

e105af3

Co-authored-by: Jeremy Rose <jeremya@chromium.org>

chore: apply PR feedback

4a36562

chore: split logic a bit

cf96382

chore: allow primitives through the world safe checl

9615819

MarshallOfSound force-pushed the world-safe-execute-js branch from 7e8bb51 to 9615819 Compare July 13, 2020 18:38

nornagon reviewed Jul 13, 2020

View reviewed changes

chore: clean up per PR feedback

fa51bf8

nornagon reviewed Jul 13, 2020

View reviewed changes

shell/renderer/api/electron_api_web_frame.cc Outdated Show resolved Hide resolved

chore: flip boolean logic

42162d2

nornagon reviewed Jul 15, 2020

View reviewed changes

lib/renderer/api/web-frame.ts Show resolved Hide resolved

lib/renderer/web-frame-init.ts Show resolved Hide resolved

shell/renderer/api/electron_api_web_frame.cc Show resolved Hide resolved

shell/renderer/api/electron_api_web_frame.cc Outdated Show resolved Hide resolved

MarshallOfSound added 3 commits July 16, 2020 11:44

chore: update per PR feedback

e407449

chore: fix typo

b82a906

chore: fix spec

149b3ba

nornagon reviewed Jul 20, 2020

View reviewed changes

nornagon approved these changes Jul 23, 2020

View reviewed changes

MarshallOfSound merged commit b500294 into master Jul 23, 2020

MarshallOfSound deleted the world-safe-execute-js branch July 23, 2020 21:32

trop bot added needs-manual-bp/8-x-y and removed target/8-x-y labels Jul 23, 2020

This was referenced Jul 23, 2020

feat: add worldSafe flag for executeJS results #24711

Merged

feat: add worldSafe flag for executeJS results #24712

Merged

trop bot removed the target/10-x-y label Jul 23, 2020

trop bot added in-flight/10-x-y and removed target/9-x-y labels Jul 23, 2020

trop bot added merged/9-x-y and removed in-flight/9-x-y labels Aug 5, 2020

ffflorian mentioned this pull request Aug 13, 2020

feat: Enable worldSafeExecuteJavaScript wireapp/wire-desktop#4135

Merged

acrosman added a commit to acrosman/ElectronSimpleStarter that referenced this pull request Aug 19, 2020

Adding world safe flag from electron/electron#24114

b86630c

bughit mentioned this pull request Oct 6, 2020

no mention in the breaking changes doc of the planned removal of worldSafeExecuteJavaScript #25796

Closed

MarshallOfSound removed the needs-manual-bp/8-x-y label Oct 21, 2020

poiru reviewed Oct 23, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add worldSafe flag for executeJS results #24114

feat: add worldSafe flag for executeJS results #24114

MarshallOfSound commented Jun 12, 2020

nornagon Jul 20, 2020

MarshallOfSound Jul 21, 2020

nornagon Jul 23, 2020

release-clerk bot commented Jul 23, 2020

trop bot commented Jul 23, 2020

trop bot commented Jul 23, 2020

trop bot commented Jul 23, 2020

bughit commented Oct 1, 2020

bughit commented Oct 5, 2020

poiru Oct 23, 2020

MarshallOfSound Oct 23, 2020

feat: add worldSafe flag for executeJS results #24114

feat: add worldSafe flag for executeJS results #24114

Conversation

MarshallOfSound commented Jun 12, 2020

nornagon Jul 20, 2020

Choose a reason for hiding this comment

MarshallOfSound Jul 21, 2020

Choose a reason for hiding this comment

nornagon Jul 23, 2020

Choose a reason for hiding this comment

release-clerk bot commented Jul 23, 2020

trop bot commented Jul 23, 2020

trop bot commented Jul 23, 2020

trop bot commented Jul 23, 2020

bughit commented Oct 1, 2020

bughit commented Oct 5, 2020

poiru Oct 23, 2020

Choose a reason for hiding this comment

MarshallOfSound Oct 23, 2020

Choose a reason for hiding this comment