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

Security Best Practices (Help Wanted) #742

Open
nklayman opened this issue May 7, 2020 · 6 comments
Open

Security Best Practices (Help Wanted) #742

nklayman opened this issue May 7, 2020 · 6 comments

Comments

@nklayman
Copy link
Owner

nklayman commented May 7, 2020

I've recently seen a handful of security related suggestions such as #733 (comment) and #610 (comment). 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
Copy link

aleksey-hoffman commented May 10, 2020

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
Copy link

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
Copy link

camhart commented Nov 24, 2020

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
Copy link

SimonEast commented Feb 25, 2021

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
Copy link

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
Copy link

reZach commented Jul 20, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants