realm / realm-js

Realm is a mobile database: an alternative to SQLite & key-value stores
https://realm.io
Apache License 2.0
5.72k stars 564 forks source link

Improve Electron support by allowing renderer process "context isolation" and disabled "node integration" #6802

Open rossicler-hostalky opened 1 month ago

rossicler-hostalky commented 1 month ago

Problem

Hi, I'm wondering if there's any plan on supporting @realm/react usage in a Electron app. I know it's possible to use with nodeIntegration: true and contextIsolation: false, but since enabling that is a security risk, I don't think it would be a good solution.

Solution

Usage of @react/realm in the renderer process without the need to use nodeIntegration: true and contextIsolation: false. Most likely by installing a renderer safe react library that would communicate with the main process to handle all updates.

Alternatives

No response

How important is this improvement for you?

Would be a major improvement

Feature would mainly be used with

Atlas Device Sync

sync-by-unito[bot] commented 1 month ago

➤ PM Bot commented:

Jira ticket: RJS-2868

kraenhansen commented 1 month ago

Thanks for your question - this is something we've had extensive conversations about. I remember @takameyer especially advocating for this, so we definitely understand the need and problem. I've updated the title removing @realm/react as I believe this request goes beyond that package.

My current understanding of Electron's choice to enable context isolation by default is primarily for apps that allow third party code to execute in the renderer. As in, apps that have an extension architecture like VS Code (or the Atom editor). Other Electron apps, will also be executing third party code through the use of dependencies, but I'd consider that within your control and therefore a calculated risk and "safer" - the same way Node.js apps use third-party code with elevated privileges. The docs on "Context Isolation" does however mention:

Context isolation has been enabled by default since Electron 12, and it is a recommended security setting for all applications.

I'd love if their recommendation was a bit more nuanced, talking about the tradeoffs involved. To me, disabling context isolation is perfectly safe if you're not planning on executing untrusted third-party code. I'd be happy to learn otherwise. Is it mainly because it's recommended that you request this or do you have other reasons that you need to disable context isolation that I might not be aware of?


From my point of view, we have two options:

Option A: Using Electron IPC primitives

For us to support this, it would require that we implement a wrapper of our native module using IPC and we it's likely that we could leverage Electron contextBridge API for this. The main drawback of any Electron IPC however, is that underneath it use message passing and serialize data as it crosses the bridge:

The api provided to exposeInMainWorld must be a Function, string, number, Array, boolean, or an object whose keys are strings and values are a Function, string, number, Array, boolean, or another nested object that meets the same conditions.

Because of this, we'd have to either implement "shallow" variants of almost all of our classes (the same way we did for the React Native debugger, which we've since happily deleted as it was a huge hazzle for us to maintain) or we could to find (or build) and rely on a more generalized RPC solution that can handle control of instances of arbitrary classes, on top of the IPC primitives provided by Electron.

Option B: Using WASM / browser binding in the Electron renderer processes

We're actively investing in stabilizing our browser support for the database running in browsers, via WebAssembly / WASM. This could be a viable alternative, with the main drawback that it could end up being hard to support sharing a database file between the renderer and main process.

rossicler-hostalky commented 1 month ago

@kraenhansen Thanks for the quick reply.

I have thought about your points when thinking about disabling, but it was mainly because of their recommendation, since it's pretty standard for you to have that enabled, and they seem to be pretty assertive about recommending to not disable it. Other than that, I understand your point, it would be awesome to not have to worry about that and just leave it enabled for that extra security, but it might not be a big problem in our case, since we do not execute untrusted party code.

As for the options you mentioned, I have thought about option A, and also tried to implement a wrapper around IPC, but for the same reason you mentioned, it would be a LOT of work to get it done and maintain, due to their limitations on primitive values. Option B seems to be the best solution if we want to keep the default electron setting regarding node integration and context isolation (at least in our use case).

Although it would be very nice to have that by default, I completely understand all your points and appreciate your detailed explanation.

One question, I see you only mentioned contextIsolation, is there any reason why you didn't mention nodeIntegration? Is it because it's a pretty similar concept or because it is not required?

kraenhansen commented 1 month ago

One question, I see you only mentioned contextIsolation, is there any reason why you didn't mention nodeIntegration? Is it because it's a pretty similar concept or because it is not required?

Thanks for asking, I realize I missed clarify that part and after some further investigations, it turns out I learned something along the way.

In the current state of the Realm JS SDK, the node integration is a must when importing realm from a renderer (except through preload scripts - more about that later). This is because we're accessing Node.js APIs to load the Realm Core binding as a Node Add-on and to access files. I had the understanding that we needed context isolation disabled for Realm to work in a renderer process, which turns out is not the case in general. Our integration tests do need this because we're relying on the require function, but if you're using a bundler then I believe realm can function just fine with context isolation enabled, as long as node integration is enabled.

If you disable node integration, the realm package can still load and function from a "preload script" of a renderer process, if the sandbox is disabled (passing sandbox: false via the webPreferences 😬), node integration can remain disabled and context isolation can remain enabled. (I just expanded our integration tests with this mode.) You would however have to implement app specific message passing to read / write data from the Realm in the actual code on the "web page", which is not ideal and won't be compatible with @realm/react as your originally requested. But .. if you disabled context isolation and passed a reference to a Realm instance through a global, that would most likely work (this I haven't tested, because our integration tests are currently built in a way where they import / require "realm"). I.e. you can probably get access to the actual Realm instance in the "web page" of a renderer process, even with node integration disabled. Using the newly published APIs to pass Realm and App instances to the providers exported from @realm/react, that might actually be feasible.

I realize we're not currently sharing these intricate details through our docs and I've alerted our docs team to help me get it published somehow.

rossicler-hostalky commented 1 month ago

That's great, having contextIsolation: false without enabling nodeIntegration wouldn't be too bad (at my point of view). I'll try it out whenever I have some time and tell you if it worked.

kraenhansen commented 1 month ago

I was thinking a bit more about this today, and I'll just add a few notes for myself (or whoever wants to pick this up).

Once #6820 gets merged, I believe it would be ideal to implement an electron platform via the electron export condition.

This could include detection of "realm" being required in an Electron renderer preload script:

  1. If in the main process, simply defer to the node platform implementation.
  2. If in a renderer preload: Inject of the native module (result of requiring "realm.node") into a global.
  3. If in a renderer process (outside of preload): It would either check for node APIs being available, in which case ir can defer to the node platform or assert the existence (printing proper instructions) of the injected global and use that instead of loading the node-addon.
rossicler-hostalky commented 1 month ago

@kraenhansen I tried it out a sample project using electron-react-boilerplate but couldn't get it working using the contextIsolation: false, nodeIntegration: false approach.

What I tried: I created a realm instance on my main process, and from the preload I tried sending the reference to the realm instance, but even with contextIsolation: false, I still couldn't send a realm instance, which rejects with Error: An object could not be cloned..

Am I missing something? I never tried sending non-serializable data from main to renderer, so I'm not sure if my usage is correct.

kraenhansen commented 1 month ago

My hypothesis involved creating the Realm instance in the renderer process as part of a preload script and then storing it on the globalThis object for the non-preload script to pick up and use.

rossicler-hostalky commented 1 month ago

My hypothesis involved creating the Realm instance in the renderer process as part of a preload script and then storing it on the globalThis object for the non-preload script to pick up and use.

Oh I see, I tried it out and was able to get the Realm instance in the renderer process with that. I had to set sandbox: false and contextIsolation: false for this to work properly.

I still had some issues of using @realm/react package in the renderer process due to how electron-react-boilerplate have different node_modules for native modules and "renderer" modules, and @realm/react peerDependencies related to react-native and/or realm (since realm is in a different package.json). But I'm pretty sure that would be doable.