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

feat: add exposeInIsolatedWorld(worldId, key, api) to contextBridge #34974

Merged
merged 3 commits into from Sep 21, 2022

Conversation

akshaydeo
Copy link
Contributor

@akshaydeo akshaydeo commented Jul 19, 2022

Description of Change

This PR adds a new function, exposeInIsolatedWorld to contextBridge. With this change, custom APIs can be exposed to Isolated worlds without any hacks when contextIsolation is enabled.

Fixes #32588

cc @miniak

Checklist

Release Notes

Notes: Added contextBridge.exposeInIsolatedWorld(worldId, key, api) to expose an API to an isolatedWorld within a renderer from a preload script.

@welcome
Copy link

welcome bot commented Jul 19, 2022

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 19, 2022
@MarshallOfSound MarshallOfSound self-requested a review July 23, 2022 08:28
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my comment above, please undo all automated formatting changes in this PR not caused by our own linter as it makes the diff almost impossible to review accurately if a lot of the changed lines don't actually contain changes

@akshaydeo
Copy link
Contributor Author

I've updated this PR to remove all lining differences

Copy link
Member

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some parts of spec-main/api-context-bridge-spec.ts still has the formatting changes.

spec-main/api-context-bridge-spec.ts Outdated Show resolved Hide resolved
spec-main/api-context-bridge-spec.ts Outdated Show resolved Hide resolved
@akshaydeo
Copy link
Contributor Author

akshaydeo commented Aug 5, 2022

The diff in the existing content from the spec file is because I had to create a separate describe blob to segregate exposeInMainWorld and exposeInIsolatedWorld scenarios 🙇‍♂️

Old structure of the spec file

contextBridge
     with useSandbox=true/false
            test case

New structure of the spec file

contextBridge
     exposeInMainWorld
         with useSandbox=true/false
            test case
     exposeInIsolatedWorld
         with useSandbox=true/false
            test case

docs/api/context-bridge.md Outdated Show resolved Hide resolved
@nornagon
Copy link
Member

nornagon commented Aug 8, 2022

Haven't reviewed the code but API LGTM.

Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

Had some feedback about the code.

shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
spec-main/api-context-bridge-spec.ts Outdated Show resolved Hide resolved
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
spec/api-context-bridge-spec.ts Outdated Show resolved Hide resolved
spec/api-context-bridge-spec.ts Outdated Show resolved Hide resolved
spec/api-context-bridge-spec.ts Outdated Show resolved Hide resolved
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is looking good. Just had a small request for tests

docs/api/context-bridge.md Outdated Show resolved Hide resolved
shell/renderer/api/electron_api_context_bridge.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@itsananderson itsananderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

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

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ckerr
Copy link
Member

ckerr commented Sep 21, 2022

Not sure what's going on with the cation bot, but this has LGTM API signoff from 3 API WG members so that is not blocking this PR

@ckerr ckerr merged commit dfc134d into electron:main Sep 21, 2022
@welcome
Copy link

welcome bot commented Sep 21, 2022

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk
Copy link

release-clerk bot commented Sep 21, 2022

Release Notes Persisted

Added contextBridge.exposeInIsolatedWorld(worldId, key, api) to expose an API to an isolatedWorld within a renderer from a preload script.

@theogravity
Copy link
Contributor

The documentation is lacking a proper example of how to use exposeInIsolatedWorld(). Do I attach the worldId to a BrowserWindow somewhere so that window has access to the isolated methods?

khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
…lectron#34974)

* feat: add exposeInIsolatedWorld(worldId, key, api) to contextBridge

* Updates exposeInIslatedWorld worldId documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: contextBridge.exposeInIsolatedWorld
9 participants