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

Already on GitHub? Sign in to your account

feat: add a new contextBridge module #20307

Merged
merged 43 commits into from Oct 18, 2019

Conversation

@MarshallOfSound
Copy link
Member

MarshallOfSound commented Sep 20, 2019

Putting this up without tests to get API and Implementation feedback. I've talked about this 1:1 with a few folks, both app devs and electron maintainers so I feel we've got a good handle here but as always, open to feedback 馃憤 Now into the fun stuff.

What is this?

This PR adds a new renderer-side module called contextBridge that allows you to expose an API synchronously from the isolated context to the main world (and optionally back again) to achieve the following goals.

  1. Make it really easy for folks currently doing the window.api = {} pattern to enable contextIsolation. The migration becomes bindAPIInMainWorld('api', {}).
  2. Make it possible for synchronous APIs to exist between the main world and the isolated world. Some app logic is heavily tied to being synchronous and rather than ask folks to refactor their entire app we can just make this possible.
  3. Kind of tied to (1) this would solve the "developer experience" issues we briefly touched on at summit around enabling contextIsolation. Once this is in, working and tested enabling contextIsolation by default no longer seems like a daunting task.

How?

The docs outline a lot of how this works from a users perspective so I'd recommend reading those. From a Behind The Scenes perspective it is all implemented using our current converter and binding infrastructure.

Primitives / Values get converted to base::Value and then converted back to v8::Value but in the other context. Functions get stored as v8::Global and then proxied into a bound cpp method that calls the original function in context A. Then does the primitive move trick and moves it to be the return value for context B. Their is special logic to proxy promise return values but you can see that both in the docs and the source code.

TODO:

  • Freeze the API object that gets bound to ensure it is immutable in the other world
    • Document that the API object is frozen and immutable
  • Function arguments need to be proxied
  • Only expose debugGC in a testing build
  • Code clean up
  • CI Tests that it works
  • CI Tests that we don't leak prototypes across the bridge
  • CI Tests that sending functions isn't a memory leak
  • Naming? Main World vs Main Process -- How do we make that clear?
  • Prevent recursion destroying us.
    • Keep a weakmap of objects as we traverse, use already proxied versions if we can
    • Throw an error if we handle a thing with a recursion depth >= 10000?
  • Use the V8 serializer - Copy from @nornagon PR and resolve conflicts with whoever lands first

Notes: Added new contextBridge module to make it easier to communicate between an isolated context and the main world

@MarshallOfSound MarshallOfSound force-pushed the context-bridge branch from c67a93e to ca9f573 Sep 20, 2019
@MarshallOfSound MarshallOfSound force-pushed the context-bridge branch 2 times, most recently from edebc37 to d1a0b44 Sep 20, 2019
@felixrieseberg

This comment has been minimized.

Copy link
Member

felixrieseberg commented Sep 20, 2019

  1. This thing will allow us to explain context isolation in a way that won't have developers wanting to look for a different job. It'll literally be night and day. I can't express enough how big of a boon for contextIsolation this is.

  2. I'm not a big fan of using mainWorld as I'm afraid that it'll get confused with the main process. I don't have a better solution right now but I'd like to brainstorm some alternatives to see if this is the best we can come up with.

### Isolated World

When `contextIsolation` is enabled in your `webPreferences` your `preload` scripts run in an "Isolated World". You can read more about
context isolation and what it affects in the [BrowserWindow](browser-window.md) docs.

This comment has been minimized.

Copy link
@codebytere

codebytere Sep 21, 2019

Member

I think since contextIsolation is mentioned in a BW constructor option and isn't super obvious from just a link to browser-window.md that it would be worthwhile to lower that barrier and re-copy it here or move it into a standalone document we could link to

docs/api/context-bridge.md Show resolved Hide resolved
lib/renderer/api/context-bridge.ts Outdated Show resolved Hide resolved
docs/api/context-bridge.md Outdated Show resolved Hide resolved
docs/api/context-bridge.md Outdated Show resolved Hide resolved
docs/api/context-bridge.md Outdated Show resolved Hide resolved
docs/api/context-bridge.md Outdated Show resolved Hide resolved
Copy link
Member

nornagon left a comment

Can we implement the primitives that would be necessary to build this as a 3rd-party library and experiment with the specific API independently of the Electron release cycle until we can settle on an API that's good? I have some reservations about the API as written (particularly regarding the "reverse bindings" part), and I don't want to enshrine this in Electron without giving it a bit of a test-drive first.

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Sep 24, 2019

Can we implement the primitives that would be necessary to build this as a 3rd-party library and experiment with the specific API independently of the Electron release cycle until we can settle on an API that's good?

To keep this answer short, no, the way this is written it requires access to v8 contexts that native modules don't have access to let alone raw JS. Also to be fair this is already very barebones, we don't prescribe API shape or structure, just "give us some functions and we'll put them in the other context".

I have some reservations about the API as written (particularly regarding the "reverse bindings" part), and I don't want to enshrine this in Electron without giving it a bit of a test-drive first.

This API is flagged as "experimental" to make it clear the API (a) is new and relatively untested and (b) may be changed in the future. If you have specific usability / API concerns I'd love to hear the specifics so that they can be addressed.

shell/renderer/api/atom_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/atom_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/atom_api_context_bridge.cc Outdated Show resolved Hide resolved
docs/api/context-bridge.md Outdated Show resolved Hide resolved
default_app/preload.ts Show resolved Hide resolved
default_app/preload.ts Show resolved Hide resolved
> Create a safe, bi-directional, synchronous bridge across isolated contexts
Process: [Renderer](../glossary.md#renderer-process)

This comment has been minimized.

Copy link
@nornagon

nornagon Sep 30, 2019

Member

Add a short paragraph explaining why the reader might want to use this. What problems does this solve?

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 15, 2019

Member

This didn't get addressed...?

docs/api/context-bridge.md Outdated Show resolved Hide resolved
docs/api/context-bridge.md Show resolved Hide resolved
shell/renderer/api/atom_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/atom_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/atom_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/atom_api_context_bridge.cc Outdated Show resolved Hide resolved
@MarshallOfSound MarshallOfSound changed the title [WIP] feat: add a new contextBridge module feat: add a new contextBridge module Oct 6, 2019
@MarshallOfSound MarshallOfSound added semver/minor and removed wip labels Oct 6, 2019
Copy link
Member

nornagon left a comment

whoops, forgot to do this as a review.

@zcbenz
zcbenz approved these changes Oct 15, 2019
namespace context_bridge {

using FunctionContextPair =
std::tuple<v8::Global<v8::Function>, v8::Global<v8::Context>>;

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 17, 2019

Member

i thought i commented on it before but it seems to have gotten lost; can we use a struct for this instead of std::tuple?

Suggested change
std::tuple<v8::Global<v8::Function>, v8::Global<v8::Context>>;
struct FunctionContextPair {
v8::Global<v8::Function> function;
v8::Global<v8::Context> context;
};
using FunctionContextPair =
std::tuple<v8::Global<v8::Function>, v8::Global<v8::Context>>;

using WeakGlobalPair = std::tuple<v8::Global<v8::Value>, v8::Global<v8::Value>>;

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 17, 2019

Member

here too

Suggested change
using WeakGlobalPair = std::tuple<v8::Global<v8::Value>, v8::Global<v8::Value>>;
struct WeakGlobalPair {
v8::Global<v8::Value> value_in_isolated_context;
v8::Global<v8::Value> value_in_main_context;
};
Copy link
Member

nornagon left a comment

Approving with above comments with the consideration that this code doesn't affect apps that don't call contextBridge.exposeInMainWorld. I still have some concerns about this implementation, but in the interests of moving this forward, let's land and iterate.

@MarshallOfSound MarshallOfSound merged commit 0090616 into master Oct 18, 2019
14 of 15 checks passed
14 of 15 checks passed
Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-woa-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20191017.19 succeeded
Details
electron-arm64-testing Build #20191017.19 succeeded
Details
electron-woa-testing Build #20191017.14 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Oct 18, 2019

Release Notes Persisted

Added new contextBridge module to make it easier to communicate between an isolated context and the main world

@MarshallOfSound MarshallOfSound deleted the context-bridge branch Oct 18, 2019
MarshallOfSound added a commit that referenced this pull request Oct 18, 2019
* feat: add a new contextBridge module

* chore: fix docs linting

* feat: add support for function arguments being proxied

* chore: ensure that contextBridge can only be used when contextIsolation is enabled

* docs: getReverseBinding can be null

* docs: fix broken links in md file

* feat: add support for promises in function parameters

* fix: linting failure for explicit constructor

* Update atom_api_context_bridge.cc

* chore: update docs and API design as per feedback

* refactor: remove reverse bindings and handle GC'able functions across the bridge

* chore: only expose debugGC in testing builds

* fix: do not proxy promises as objects

* spec: add complete spec coverage for contextBridge

* spec: add tests for null/undefined and the anti-overwrite logic

* chore: fix linting

* spec: add complex nested back-and-forth function calling

* fix: expose contextBridge in sandboxed renderers

* refactor: improve security of default_app using the new contextBridge module

* s/bindAPIInMainWorld/exposeInMainWorld

* chore: sorry for this commit, its a big one, I fixed like everything and refactored a lot

* chore: remove PassedValueCache as it is unused now

Values transferred from context A to context B are now cachde in the RenderFramePersistenceStore

* chore: move to anonymous namespace

* refactor: remove PassValueToOtherContextWithCache

* chore: remove commented unused code blocks

* chore: remove .only

* chore: remote commented code

* refactor: extract RenderFramePersistenceStore

* spec: ensure it works with numbered keys

* fix: handle number keys correctly

* fix: sort out the linter

* spec: update default_app asar spec for removed file

* refactor: change signatures to return v8 objects directly rather than the mate dictionary handle

* refactor: use the v8 serializer to support cloneable buffers and other object types

* chore: fix linting

* fix: handle hash collisions with a linked list in the map

* fix: enforce a recursion limit on the context bridge

* chore: fix linting

* chore: remove TODO

* chore: adapt for PR feedback

* chore: remove .only

* chore: clean up docs and clean up the proxy map when objects are released

* chore: ensure we cache object values that are cloned through the V8 serializer
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Oct 18, 2019

@MarshallOfSound has manually backported this PR to "6-0-x", please check out #20639

@trop trop bot added the in-flight/6-0-x label Oct 18, 2019
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Oct 21, 2019

@MarshallOfSound has manually backported this PR to "6-1-x", please check out #20639

@trop trop bot added the in-flight/6-1-x label Oct 21, 2019
MarshallOfSound added a commit that referenced this pull request Oct 21, 2019
* feat: add a new contextBridge module (#20307)

* feat: add a new contextBridge module

* chore: fix docs linting

* feat: add support for function arguments being proxied

* chore: ensure that contextBridge can only be used when contextIsolation is enabled

* docs: getReverseBinding can be null

* docs: fix broken links in md file

* feat: add support for promises in function parameters

* fix: linting failure for explicit constructor

* Update atom_api_context_bridge.cc

* chore: update docs and API design as per feedback

* refactor: remove reverse bindings and handle GC'able functions across the bridge

* chore: only expose debugGC in testing builds

* fix: do not proxy promises as objects

* spec: add complete spec coverage for contextBridge

* spec: add tests for null/undefined and the anti-overwrite logic

* chore: fix linting

* spec: add complex nested back-and-forth function calling

* fix: expose contextBridge in sandboxed renderers

* refactor: improve security of default_app using the new contextBridge module

* s/bindAPIInMainWorld/exposeInMainWorld

* chore: sorry for this commit, its a big one, I fixed like everything and refactored a lot

* chore: remove PassedValueCache as it is unused now

Values transferred from context A to context B are now cachde in the RenderFramePersistenceStore

* chore: move to anonymous namespace

* refactor: remove PassValueToOtherContextWithCache

* chore: remove commented unused code blocks

* chore: remove .only

* chore: remote commented code

* refactor: extract RenderFramePersistenceStore

* spec: ensure it works with numbered keys

* fix: handle number keys correctly

* fix: sort out the linter

* spec: update default_app asar spec for removed file

* refactor: change signatures to return v8 objects directly rather than the mate dictionary handle

* refactor: use the v8 serializer to support cloneable buffers and other object types

* chore: fix linting

* fix: handle hash collisions with a linked list in the map

* fix: enforce a recursion limit on the context bridge

* chore: fix linting

* chore: remove TODO

* chore: adapt for PR feedback

* chore: remove .only

* chore: clean up docs and clean up the proxy map when objects are released

* chore: ensure we cache object values that are cloned through the V8 serializer

* docs: mark contextBridge as experimental (#20638)

* docs: mark contextBridge as experimental

This commit didn't make it to the original PR, quick addition here

* Update context-bridge.md

* chore: touch up the differences between master and 6-0-x

* chore: add v8 serializer converter, cherry picked from 2fad53e

* chore: support converting OnceCallback to V8 (#17941)

* chore: fixup tests

* chore: fix linting

* chore: add patch for mojo message constructor
@trop trop bot added merged/6-1-x and removed in-flight/6-1-x labels Oct 21, 2019
@sofianguy sofianguy added this to Fixed in 6.1.0 in 6.1.x Oct 22, 2019
MarshallOfSound added a commit that referenced this pull request Oct 28, 2019
* feat: add a new contextBridge module

* chore: fix docs linting

* feat: add support for function arguments being proxied

* chore: ensure that contextBridge can only be used when contextIsolation is enabled

* docs: getReverseBinding can be null

* docs: fix broken links in md file

* feat: add support for promises in function parameters

* fix: linting failure for explicit constructor

* Update atom_api_context_bridge.cc

* chore: update docs and API design as per feedback

* refactor: remove reverse bindings and handle GC'able functions across the bridge

* chore: only expose debugGC in testing builds

* fix: do not proxy promises as objects

* spec: add complete spec coverage for contextBridge

* spec: add tests for null/undefined and the anti-overwrite logic

* chore: fix linting

* spec: add complex nested back-and-forth function calling

* fix: expose contextBridge in sandboxed renderers

* refactor: improve security of default_app using the new contextBridge module

* s/bindAPIInMainWorld/exposeInMainWorld

* chore: sorry for this commit, its a big one, I fixed like everything and refactored a lot

* chore: remove PassedValueCache as it is unused now

Values transferred from context A to context B are now cachde in the RenderFramePersistenceStore

* chore: move to anonymous namespace

* refactor: remove PassValueToOtherContextWithCache

* chore: remove commented unused code blocks

* chore: remove .only

* chore: remote commented code

* refactor: extract RenderFramePersistenceStore

* spec: ensure it works with numbered keys

* fix: handle number keys correctly

* fix: sort out the linter

* spec: update default_app asar spec for removed file

* refactor: change signatures to return v8 objects directly rather than the mate dictionary handle

* refactor: use the v8 serializer to support cloneable buffers and other object types

* chore: fix linting

* fix: handle hash collisions with a linked list in the map

* fix: enforce a recursion limit on the context bridge

* chore: fix linting

* chore: remove TODO

* chore: adapt for PR feedback

* chore: remove .only

* chore: clean up docs and clean up the proxy map when objects are released

* chore: ensure we cache object values that are cloned through the V8 serializer
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Oct 28, 2019

@MarshallOfSound has manually backported this PR to "7-0-x", please check out #20789

@trop trop bot added the in-flight/7-0-x label Oct 28, 2019
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 4, 2019

@MarshallOfSound has manually backported this PR to "7-1-x", please check out #20789

@trop trop bot added in-flight/7-1-x and removed in-flight/7-1-x labels Nov 4, 2019
MarshallOfSound added a commit that referenced this pull request Nov 4, 2019
* feat: add a new contextBridge module (#20307)

* feat: add a new contextBridge module

* chore: fix docs linting

* feat: add support for function arguments being proxied

* chore: ensure that contextBridge can only be used when contextIsolation is enabled

* docs: getReverseBinding can be null

* docs: fix broken links in md file

* feat: add support for promises in function parameters

* fix: linting failure for explicit constructor

* Update atom_api_context_bridge.cc

* chore: update docs and API design as per feedback

* refactor: remove reverse bindings and handle GC'able functions across the bridge

* chore: only expose debugGC in testing builds

* fix: do not proxy promises as objects

* spec: add complete spec coverage for contextBridge

* spec: add tests for null/undefined and the anti-overwrite logic

* chore: fix linting

* spec: add complex nested back-and-forth function calling

* fix: expose contextBridge in sandboxed renderers

* refactor: improve security of default_app using the new contextBridge module

* s/bindAPIInMainWorld/exposeInMainWorld

* chore: sorry for this commit, its a big one, I fixed like everything and refactored a lot

* chore: remove PassedValueCache as it is unused now

Values transferred from context A to context B are now cachde in the RenderFramePersistenceStore

* chore: move to anonymous namespace

* refactor: remove PassValueToOtherContextWithCache

* chore: remove commented unused code blocks

* chore: remove .only

* chore: remote commented code

* refactor: extract RenderFramePersistenceStore

* spec: ensure it works with numbered keys

* fix: handle number keys correctly

* fix: sort out the linter

* spec: update default_app asar spec for removed file

* refactor: change signatures to return v8 objects directly rather than the mate dictionary handle

* refactor: use the v8 serializer to support cloneable buffers and other object types

* chore: fix linting

* fix: handle hash collisions with a linked list in the map

* fix: enforce a recursion limit on the context bridge

* chore: fix linting

* chore: remove TODO

* chore: adapt for PR feedback

* chore: remove .only

* chore: clean up docs and clean up the proxy map when objects are released

* chore: ensure we cache object values that are cloned through the V8 serializer

* docs: mark contextBridge as experimental (#20638)

* docs: mark contextBridge as experimental

This commit didn't make it to the original PR, quick addition here

* Update context-bridge.md

* chore: update for 7-0-x differences

* chore: update callback header

* chore: add v8 serializer converter, cherry picked from 2fad53e

* chore: update for 7-0-x differences
@trop trop bot added the merged/7-1-x label Nov 4, 2019
@sofianguy sofianguy added this to Fixed in 7.1.0 in 7.1.x Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.