nklayman / vue-cli-plugin-electron-builder

Easily Build Your Vue.js App For Desktop With Electron
https://nklayman.github.io/vue-cli-plugin-electron-builder/
MIT License
4.12k stars 280 forks source link

Security Best Practices (Help Wanted) #742

Open nklayman opened 4 years ago

nklayman commented 4 years ago

I've recently seen a handful of security related suggestions such as https://github.com/nklayman/vue-cli-plugin-electron-builder/issues/733#issuecomment-623763139 and https://github.com/nklayman/vue-cli-plugin-electron-builder/issues/610#issuecomment-618901558. With v2.0 of this plugin I'd like to make it much more secure by default. I've started by disabling nodeIntegration by default, but I'd also like to add a security section to the docs.

If you have any suggestions related to security, whether it be something I should change in the background.js template or something I should explain in this new section, please leave a comment below. As a bonus, I'd really appreciate a short tutorial on how to configure it if the process isn't as simple as changing a line or two of code. Thank you for your help making the many apps built with this plugin more secure.

EDIT: The security page is now live, but I'll leave this open as I could still add more to it.

aleksey-hoffman commented 4 years ago

Issue: webPreferences.webSecurity

When you try to display images or other files in a renderer window with webSecurity: true you get the following error: Not allowed to load local resource.

Most answers on the internet tell you to just set webSecurity: false, so I would imagine that's exactly what most devs do. It solves the problem and allows you to load images, but it makes the app less safe to use.

I don't know how dangerous it actually is to use an app with disabled webSecurity, but according to the docs and to Electron maintainer @MarshallOfSound, disabling webSecurity is a bad idea:

loading arbitrary local resources from a remote endpoint is a massive security violation and you should be aware of what you are doing if you intend for that to be possible.

The correct way to handle this kind of thing is to expose a custom protocol handler app:// that can serve specific files from local disk for your remote endpoint to use.

Solution

To make electron-builder v2.0 safer by default, and prevent developers from monkey-patching their apps with webSecurity: false, perhaps electron-builder should regitster a custom protocol automatically:

Main process:

import { protocol } from 'electron'
...

app.on('ready', () => {
  registerSafeFileProtocol()
  ...
})

function registerSafeFileProtocol() {
  const safeFileProtocol = `${appName}-safe-file-protocol`
  protocol.registerFileProtocol(safeFileProtocol, (request, callback) => {
    const url = request.url.replace(`${safeFileProtocol}://`, '')
    // Decode URL to prevent errors when loading filenames with UTF-8 chars or chars like "#"
    const decodedUrl = decodeURIComponent(url)
    try {
      return callback(decodedUrl)
    }
    catch (error) {
      console.error('ERROR: main | registerSafeFileProtocol | Could not get file path', error)
    }
  })
}

I think it should add the app name to the protocol: ${appName}-safe-file-protocol, otherwise, if you have multiple apps that use the same protocol, there might be a conflict, for example, if you have 2 apps with this:

app.setAsDefaultProtocolClient('safe-file-protocol')

And then open a file from a URL using the protocol, e.g. safe-file-protocol://test.jpg - will all the apps with that protocol open? Will you get an error?

Docs

Also docs would have to be updated to explain how to use the protocol to load files:

Method 1: load image in a renderer window:

Renderer process:

<img :src="getSafePath('C:/test.jpg')">
computed: {
  getSafePath (path) {
    // return `${__safeFileProtocol}://${path}`
    return `appName-safe-file-protocol://${path}`
  }
}

Method 2: load path selected by user via dialog from the main process:

Main process:

ipcMain.on('open-file-dialog', (event) => {
  const window = BrowserWindow.getFocusedWindow()

  dialog.showOpenDialog(window, { properties: ['openFile'] })
    .then(result => {
      // Send the path back to the renderer
      event.sender.send('open-file-dialog-reply', { path: result.filePaths[0] })
    })
    .catch(error => {
       console.log('ERROR: main | open-file-dialog | Could not get file path')
    })
})

Renderer process:

<img id="image-1">
mounted () {
  ipcRenderer.on('open-file-dialog-reply', (event, data) => {
    this.loadImage(data.path)
  }
},
methods: {
  loadImage (path) {
    const image1 = document.getElementById('image-1')
    image1.src = `appName-safe-file-protocol://${path}`
  }
}
Sparkenstein commented 4 years ago

Following comments from #610 someone made a electron template for react that targets security only. take a look at secure-electron-template). preload.js from this project is a good example of how to expose protected methods.

camhart commented 3 years ago

I have multiple pages that I load in BrowserWindow's. Some require nodeIntegration, some do not. It seems like the current patch for vue.config.js only allows it to be set globally. Is it possible to have it set on a per-entry level?

  pluginOptions: {
    electronBuilder: {
      nodeIntegration: true, // needs to be done on a per entry level

Here's what I need:

  pages: {
    index: 'src/main.js',  //nodeIntegration = true, only serves local content
    worker: 'src/Worker.js', //nodeIntegration = true, only serves local content
    auth: 'src/BrowserWindowAuth.js' //nodeIntegration = false, as this page loads a remote oauth page
  },
SimonEast commented 3 years ago

I think the nodeIntegration setting should be strongly avoided and discouraged. Instead preload files should be the main method for accessing Node APIs and packages. The current documentation hints at this, but I think it could be clearer with a more complete example. Here's a draft I've put together based on a combination of Electron's docs and VCPEB docs. Perhaps you could add this to official docs if you agree with the approach.

Safely Accessing Node APIs

Although it's sometimes mentioned, the global nodeIntegration setting should NOT be enabled as this allows the renderer to require(...) almost anything and have full access to file system (see here), which is very dangerous for security.

The safer alternative is to use a preload file whereby you expose your own functions that use the Node API. Here's an example:

// vue.config.js

module.exports = {
  pluginOptions: {
    electronBuilder: {
      preload: 'src/preload.js',
      // Or, for multiple preload files:
      // preload: { preload: 'src/preload.js', otherPreload: 'src/preload2.js' }
    }
  }
}
// src/background.js (short snippet)

// Create the browser window.
const win = new BrowserWindow({
  webPreferences: {   
    // Use pluginOptions.nodeIntegration, leave this alone
    // See nklayman.github.io/vue-cli-plugin-electron-builder/guide/security.html#node-integration for more info
    // nodeIntegration: process.env.ELECTRON_NODE_INTEGRATION,
    preload: __dirname + '/preload.js',
  }
})
// src/preload.js

// In this file, we can call Node APIs and packages via require() without 
// exposing our entire system to the renderer. We only expose specific functions.
// Remember to handle all user-input safely so that these functions cannot be abused
// against their original intention.

const fs = require('fs');

window.myFileSystemOperation = function() {
    const folders = fs.readdirSync('/');
    console.log(folders);   
}

With that code in place, we can now call myFileSystemOperation() in our client-side Javascript/Vue code in a safer way that does not expose the entire Node API to the renderer.

skulls-dot-codes commented 2 years ago

The docs for nodeIntegration with ipcRenderer or contextBridge should be updated: https://nklayman.github.io/vue-cli-plugin-electron-builder/guide/security.html#node-integration

While your example is OK for local rendering, there should be a disclaimer for loading remote urls, or use the "Good Code" example: https://www.electronjs.org/docs/latest/tutorial/context-isolation#security-considerations

reZach commented 2 years ago

@Sparkenstein / @aleksey-hoffman (Author of secure-electron-template here). I've written a blog post describing how one can load images in development and production environments (which is also secure) - in case this is something you are still looking answers for.