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

bind DiskFileSystemProvider for file scheme in electron-renderer process #8177

Open
akosyakov opened this issue Jul 15, 2020 · 7 comments
Open
Labels
electron issues related to the electron target filesystem issues related to the filesystem performance issues related to performance security issues related to security

Comments

@akosyakov
Copy link
Member

akosyakov commented Jul 15, 2020

Motivation:#8152 (comment)

There are following prerequisites:

  • opening large or binary files #8152 has to be landed
  • We should learn to bundle Node.js (native) modules, since DiskFileSystemProvider makes use of path, fs and nsfw
    modules. It could be done as a separate PR which bundles the backend and the same approach should be applied to frontend code for electron case.
  • From security perspective we would like to enable node isolation, but it won't be possible to use DiskFileSystemProvider like that then. We should figure out why VS Code allows it and when enabling node isolation actually make sense.
@akosyakov akosyakov added filesystem issues related to the filesystem electron issues related to the electron target performance issues related to performance security issues related to security labels Jul 15, 2020
@akosyakov
Copy link
Member Author

cc @marcdumais-work @marechal-p

@paul-marechal
Copy link
Member

https://webpack.js.org/configuration/target/

Seems like the electron-renderer target includes a bunch of useful plugins, such as NodeTargetPlugin:

NodeTargetPlugin

node/NodeTargetPlugin()

The plugins should be used if you run the bundle in a Node.js environment.

If ensures that native modules are loaded correctly even if bundled.

@paul-marechal
Copy link
Member

paul-marechal commented Jul 16, 2020

Regarding the security concern of leaving nodeIsolation: true: it is not recommended for sure, but it looks painful to disable in our case indeed. I was reading about Electron's contextIsolation parameter with preload scripts, and maybe something can be done:

  • We enable both node isolation and context isolation, we start making use of preload scripts.
  • We expose from preload all the class implementations that use the Node APIs, but we don't directly expose the Node APIs.
  • We bind those implementations using inversify from the actual frontend code (running without node integration).

If we do not disable nodeIntegration, we expose users to RCEs or worse exploits if someone finds a way to trigger an XSS injection while the app is running.

On the other hand... We provide components (fs, processes, etc) that do almost everything that node does. Hiding node but leaving our own APIs that are just as powerful doesn't help in that regard...

@marcdumais-work
Copy link
Contributor

On the other hand... We provide components (fs, processes, etc) that do almost everything that node does. Hiding node but leaving our own APIs that are just as powerful doesn't help in that regard...

What are the attack vector(s), for exploiting the node integration and Theia's APIs? i.e. where would that bad code potentially come from? I can see a few sources:

  • Malicious Theia extension
  • Malicious VS Code extension
  • Malicious runtime dependency (we have ~1000+ production dependencies included in the example app)
  • Code injected in app by some exploit, potentially remotely

Other ideas?

@marcdumais-work
Copy link
Contributor

@marechal-p ping

@paul-marechal
Copy link
Member

paul-marechal commented Jul 20, 2020

I don't have other ideas. The first 3 points are part of the build system, before publishing. The last point (XSS injection) can happen if some exploit is present in our own code, or some library (but mostly in our own code).

@akosyakov
Copy link
Member Author

I'm particularly interested in VS Code view on this topic as well. It should be the resolved issue for them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron issues related to the electron target filesystem issues related to the filesystem performance issues related to performance security issues related to security
Projects
None yet
Development

No branches or pull requests

3 participants