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 ElectronContextBridgeProvider #2914

Conversation

trulysinclair
Copy link
Contributor

@trulysinclair trulysinclair commented Dec 13, 2023

Closes #2618

PR Type

  • Feature

Description of the changes

This PR adds the ElectronContextBridgeProvider which adds support for Electron applications that use the isolated context and preload scripts. Developers using this provider MUST follow the steps included in the README to set up a contextBridge that correctly implements the IContextBridgeImpl interface. I've opted to pass the defined contextBridge object to the provider so that the developer is able to freely name the context bridge api without being forced to use "main" or "electronApi", etc.

PR checklist

  • Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
  • All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
  • Stories have been added and existing stories have been tested
  • Added appropriate documentation. Docs PR:
  • License header has been added to all new source files (yarn setLicense)
  • Contains NO breaking changes

Other information

@trulysinclair trulysinclair requested a review from a team as a code owner December 13, 2023 01:35
@trulysinclair
Copy link
Contributor Author

@microsoft-github-policy-service agree

@gavinbarron
Copy link
Member

This is excellent stuff and much needed, so thank you @trulysinclair

What do you think of bumping to a recent version of Electron?

@trulysinclair
Copy link
Contributor Author

trulysinclair commented Dec 15, 2023

Thank you, and the only usage of Electron is BrowserWindow, ipcMain, ipcRenderer, and protocol. The current version is 28 stable, and currently 11 is in use. These four APIs haven't seen any breaking changes that would conflict from what I've seen, so I'd say its safe to upgrade to the latest version.

@trulysinclair
Copy link
Contributor Author

trulysinclair commented Dec 16, 2023

This may need to be halted, while the context bridge and everything works fine, attempting to use the react wrapper causes issues with 'require' is not defined, or not being able to reach into node_modules. For this context provider to work fully, those components would also need to work without nodeIntegration set to true.

This was a massive oversight on my end, I apologize.

EDIT: Never mind, the issue was the Content Security Policy not allowing MGT to fetch the necessary URLs. Easily fixed.

@sebastienlevert
Copy link
Contributor

@trulysinclair thanks for this PR! It's amazing to see this, I am sure our Electron devs will be very thankful for this great change! It may take some time because of the Holiday season to process, but this is not forgotten! I added the related bug to out v4 milestone that will land in early 2024, so not only this will be fixed, but it will be part of a great major release! I'll let the engineers review and comment on the PR, but I'm excited for this scenario! Thanks!

Copy link
Member

@gavinbarron gavinbarron left a comment

Choose a reason for hiding this comment

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

This is great stuff!

Thank you so much for providing this improvement @trulysinclair

@gavinbarron gavinbarron enabled auto-merge (squash) January 3, 2024 18:36
@gavinbarron gavinbarron merged commit 8900eb4 into microsoftgraph:main Jan 3, 2024
@trulysinclair
Copy link
Contributor Author

This is great stuff!

Thank you so much for providing this improvement @trulysinclair

Thank you! I'm glad I could contribute to this awesome toolkit!

@gavinbarron
Copy link
Member

Hey @trulysinclair are you able to help out with a sample app on this?

I've been trying to get this running and keep running into issues with electron that I'm sure a developer experienced with electron would solve quickly, but I'm not that person and would greatly appreciate the help on this one.

@trulysinclair
Copy link
Contributor Author

@gavinbarron absolutely, I've been using it in production I can extract what works for a sample app

@gavinbarron
Copy link
Member

@trulysinclair Thank you, perhaps we could PR it to replace the existing https://github.com/pnp/mgt-samples/tree/main/samples/app/electron-app

@trulysinclair
Copy link
Contributor Author

trulysinclair commented Jan 26, 2024

@gavinbarron Would you prefer a second one, that way both with and without contextBridge exist for devs who may opt out of isolation for prototyping?

@gavinbarron
Copy link
Member

That would be fantastic! Although part of me thinks we should be guiding folks towards the more secure and preferred ContextBridge approach

@trulysinclair
Copy link
Contributor Author

That's a good point, I'll rewrite the existing unsafe example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG] Electron support broken when ContextIsolation is needed.
3 participants