-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
[Future-waiting on Electron framework] Implement sandbox #10
Comments
Sandboxing will means deprecrating the IPC architecture in favor of ProtocolHandlers. |
@kewde Are you implying that there has been a push/announcement from the Electron team that protocol handlers are the best way moving forward, or is this just an assumption? I would be most comfortable moving in the direction that the Electron team recommends. |
The |
@lacymorrow Where are you seeing this being implemented? I still see that it is experimental: https://www.electronjs.org/docs/api/sandbox-option#status |
It's implemented & available in many releases, just marked as experimental. |
@kewde It think that's his whole point. I agree. Something so explicitly marked as experimental is likely not production-ready. |
I have used it in production for over two years and haven't had any issues. At this point I'm willing to believe it's marked as experimental just because they don't want to claim it is secure. |
Yep, it's in production builds is my point. I understand the argument for waiting until it's not labeled experimental, however. |
@lacymorrow , @kewde , @b-zurg - I've commented on this electron issue and hopefully we can get an answer from the team on the status of this feature. |
@lacymorrow @kewde @b-zurg it looks like it's officially supported, that's good. There's one barrier as of today to update this template to support sandbox mode and that's to change The problem is this line which is used to get initial store data synchronously for users who need access to their store data when their app loads. I am open to some help, if any of you have time. |
I would suggest using moving away from any IPC-based design as it does not scale very well for large applications. The protocol handlers are likely a better option as they use the same network stack as a normal HTTP request (as far as I understand). https://chromium.googlesource.com/chromium/src/+/master/net/docs/life-of-a-url-request.md An asynchronous call can be made synchronous so I don't think there is much of an issue there. |
@kewde Understood, I believe you made that point with regards to IPC earlier in this issue. What particular approach in here are you recommending? It's not just that, but the store module requires |
I referred to the article to explain that IPC does not perform well for a lot of data, therefore protocol handlers are a better alternative. Write a ProtocolHandler for example "securefs://".
I do not recommend JSON as the actual underlying protocol IDL. It's better to pass binary messages, because JSON can not carry binary data (only when BASE64 encoded). Then rewrite the Store to use the protocol handlers. |
My 2 cents, do the above but write a generic variant. in main process:
Every request passed to "securecomm://fs" will go through whitelistFS. |
How high performance are you thinking of when you made this comment. Are you desiring to make apps in this way or to make this template more performant? |
@kewde What I was trying to tease out is that my intention is to keep this repository aligned with the methods the Electron team recommends, instead of rolling my own protocol, unless of course that change is minimal and isn't a large or unfamiliar or frequently touched change. I'd be more than fine you taking this repo and making a higher-performance fork of it if you'd like. |
Edit, misunderstood. Re-opening. |
Hi @reZach 👋 Thanks for this very helpful repo! I see that Electron's BackgroundA simplified version of my configuration (using Electron 17.0.0) is something like this:
And that's all working as expected (thanks to the guidance from this repo 🙏 ). The problemIf I enable
What I've tried to fix thisRemove my use of
|
@roryabraham Thanks! I'm glad this repo helped you along. |
Sandbox will be a property on the BrowserWindow in the future (?) hopefully. When it is in place, enhance the template to use it. Using sandbox will force ipc communication between the renderer and main processes. Even though that's how the template is built now, it feels better to force our decisions by the framework than by our own designs.
When this is implemented, copy the documentation from here and write up some documentation to verify we are in sandbox mode.
The text was updated successfully, but these errors were encountered: