min icon indicating copy to clipboard operation
min copied to clipboard

Change contextIsolation to true by default

Open glitsj16 opened this issue 5 years ago • 3 comments

Can you describe the problem you're facing?

After seeing this electron discussion I noticed that min code uses contextIsolation: false in main.js. To be clear, min browser is working fine with the current webPreferences.

What solution would you like?

I'm not fully familiar with the electron code base, nor am I a professional security expert. Just wondering whether this important, security-related setting could be enabled by default in min code (without any loss of functionality). Info on potential issues when doing so are very welcome too.

Are there any alternatives you've considered?

I have been running a custom min build with contextIsolation: true and as far as I can ascertain things work just as fine. Again, this is a feature request / request for opinions on how changing this setting might affect user (sense of) security.

glitsj16 avatar Dec 13 '20 22:12 glitsj16

Yeah, this is a good idea to fix. To summarize how Min currently works:

  • Web pages you load are sandboxed, and don't have any access to Node (sandbox: true, contextIsolation: true)
  • The Min UI is another web page, and its code has full Node access (contextIsolation: false).

There are a bunch of things in the Min UI that require node access to work. Examples:

  • Settings (uses fs to load the settings file)
  • Userscripts (reads files from the userscripts directory)
  • The password manager (which launches a subprocess)
  • Things like this code.

Actually, if I enable contextIsolation and rebuild, Min doesn't even launch for me - maybe you didn't rebuild it?

The risk with the current setup is that if there was an XSS issue in the Min UI code, it might be possible to run JS that would have full access to your computer. This is somewhat mitigated by the fact that we have a content-security-policy for the UI page that should prevent injected scripts from running. Also, I don't think we ever display untrusted HTML anywhere, and generally use textContent for everything. But it's definitely still something we should address.

The solution to this would be to move code that needs node access into a preload script that communicates with the UI, and then able contextIsolation. That way, any XSS vulnerability would be less severe, as it would only be possible to access predefined files (like the settings file) that are normally used, rather than having full disk access.

PalmerAL avatar Dec 14 '20 03:12 PalmerAL

Thank you for the very informative response.

Actually, if I enable contextIsolation and rebuild, Min doesn't even launch for me - maybe you didn't rebuild it?

I made a mess of a local patch and can confirm that simply setting contextIsolation to true in the Min UI does not work with the current code base.

glitsj16 avatar Dec 14 '20 12:12 glitsj16

In order to make the app work while context isolation is enabled, you need to use ipc and contextBridge wherever it's necessary. (Source)

KaKi87 avatar Jun 23 '22 11:06 KaKi87