feat: use contextBridge in favour of requiring nodeIntegration

[matmalkowski]

Due to security concerns related to usage of nodeIntegration flag, according to best electron practices, renderer functions should be exposed with contextBridge. This PR does exactly that. It also changes a bit API to accomodate for this feature

[Nantris]

Do any of the functions exposed in the bridge allow a user to potentially gain access to the current store state, or any of the underlying BrowserWindow objects? Like I think fetchInitialState would be dangerous to expose as a global value. This could be worked around with the sort of solution used for preventDoubleInitialization but then that creates more problems if a user wants to reload the Redux environment without starting.

I think also exposing subscribeToIPCAction could be dangerous since it would let a malicious user pass any callback they wish to that function and read the data.

---

If I'm not mistaken about the dangers of exposing those functions, an alternative method would be a bindings approach, but I have to warn I've not actually tried implementing this for any library I've worked with yet.

I got the idea from: https://github.com/reZach/secure-electron-context-menu/blob/master/src/index.js

and then it could be used like this: https://github.com/reZach/secure-electron-context-menu#modify-your-preloadjs-file - although in the case of Electron-Redux I think we wouldn't need to expose anything via contextBridge except for composeWithStateSync - which I think is safe to expose?

What do you think?

[Nantris]

We're just finishing up a release cycle here and starting to think about this again. @matmalkowski any thoughts on a path forward?

[matmalkowski]

I got some internal review already, but I still don't understand the suggested bindings approach and how it would change the security vs this implementation. Would be great if you could provide some example with the difference in behavior we would see @slapbox

[Nantris]

Happy to discuss @matmalkowski!

The overarching concern with this implementation is the exposure of potentially sensitive or dangerous methods in the global scope.

fetchInitialState

The first thing that stands out to me is exposing fetchInitialState. Because this would end up mapped somewhere like window.api.fetchInitialState. If malicious code gained access within the application, exposing the entire state in the global scope gives the attackers free reign to access any and all data, at least the initial values.

Since Redux is supposed to be the "single source of truth," it inherently has a large portion of the application's data. The danger of this is going to depend on what sort of data is stored in the store, of course. Personal data, health data, financial data, etc., would ideally never be accessible from a global scope.

With the bindings approach, no functions to access the store's state would be exposed in a global scope which would make access to the store's state dramatically more difficult for an attacker.

---

subscribeToIPCAction

There's also probably a risk to having subscribeToIPCAction in the global scope. This potential attack I'll outline is just spitballing and could maybe be mitigated through other mechanisms, but there's sure to be other similar attacks someone more imaginative and intent on doing harm could find.

A potential attack I could envision is something like: an attacker could potentially register a function to send the action's payload via an xhr request whenever the IPC channel triggers, which for this particular IPC channel will be for every single Redux action. This would let an attacker access not just the initial state, but also have ongoing access to how the state develops over time.

The fact that the event arg is not being passed from the IPC channel here is very good to prevent access to Electron internals, but it still leaves application-specific vulnerabilities.

With that in mind, it appears my concern about a BrowserWindow potentially being exposed is moot.

---

The current Electron-Redux (master branch and alpha branch) don't allow attacks of either sort, so it seems like introducing access to those functions in the name of improving Electron security would be trading one type of risk for another.

I was very glad to see the Redux store removed from the global state in the main process on the alpha branch. It was my only security concern in the master branch, since it was trivial to access if an attacked managed to access the remote functionality and ran remote.getGlobal('getReduxState')(). The "hard" part of that attack was gaining access to remote. Putting the ability to access the state in the global scope of the renderer process makes it easier for an attacker to access the data than for them to need to gain access to the global scope of the main process.

---

With the bindings approach I think the only thing that would need to be exposed is composeWithStateSync which I can't imagine any potential attack vector for at this moment, but again, maybe a lack of imagination. If there is an attack vector involving composeWithStateSync then maybe there's no way to entirely prevent putting something dangerous in the global scope, but even if that's the case, the bindings approach would reduce the attack surface a lot. Is there anywhere in composeWithStateStync that an attacker would be able to access the store state or register a malicious action to the IPC listener that I'm overlooking?

[matmalkowski]

Thanks for the writeup - I agree, the security risk created by this approach create actual regression and are not desired. I just didn't understand the bindings approach because I got confused by the idea that binding some functions will make things different from scope perspective. But the issues are basically addressed by making sure only the right functions are exposed, instead of the more raw ones. I took your example and moved to expose in preload script only the 2 required higher level functions as in composeWithStateSync and stateSyncEnhancer. I will commit the change in a sec, once I check the app is working as expected

[matmalkowski]

@slapbox so is 5c1b94e what we are looking for? :)

[Nantris]

@matmalkowski I think this looks good! I'm sorry for the confusion I've caused by overthinking this/thinking that a minor tweak couldn't achieve the same end. I'm going to blame that on lack of intuition about how it all works together, though that's a pretty weak excuse!

I think that the only remaining point, from my perspective, is to make sure everyone agrees those functions don't expose any potential dangers. I don't see any potential dangers myself. I'm pretty sure that composeWithStateSync doesn't return anything potentially valuable to an attacker.

[Nantris]

Not a security thing, but would supporting a custom value here, in place of __ElectronReduxBridge, require changes to anything besides the preload function? I did a quick search and didn't see anything else that would need updating to support that.

By the way, thank you for your great work on this issue!

[matmalkowski]

Not a security thing, but would supporting a custom value here, in place of __ElectronReduxBridge, require changes to anything besides the preload function? I did a quick search and didn't see anything else that would need updating to support that.

I don't know, probably could be done (but not as part of this PR though - what would be use case for it?

[Nantris]

Really just a curiosity on my side/a developer experience thing, I shouldn't even have raised the distraction.

I can envision a future where a year from now the window object has 20 different keys added to it by various projects so I'm just trying to think ahead about that.

[victorhqc]

Looks good to me! Just a small question about the ipcRenderer

[matmalkowski]

🎉 This PR is included in version 2.0.0-alpha.9 🎉

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀