Fix elevation on windows when the path contains "&" or "'"

Change-type: patch
Changelog-entry: Fix elevation on windows when the path contains "&" or "'"

lib/gui/app/os/windows-network-drives.js

@@ -23,59 +23,9 @@ const _ = require('lodash')
 const os = require('os')
 const Path = require('path')
 const process = require('process')
-const tmp = require('tmp')
 const { promisify } = require('util')
 
-/**
- * @summary returns { path: String, cleanup: Function }
- * @function
- *
- * @returns {Promise<{ path: String, cleanup: Function }>}
- *
- * @example
- * tmpFileAsync()
- *   .then({ path, cleanup } => {
- *     console.log(path)
- *     cleanup()
- *   });
- */
-const tmpFileAsync = () => {
-  return new Promise((resolve, reject) => {
-    const options = {
-
-      // Close the file once it's created
-      discardDescriptor: true,
-
-      // Wmic fails with "Invalid global switch" when the "/output:" switch filename contains a dash ("-")
-      prefix: 'tmp'
-    }
-    tmp.file(options, (error, path, _fd, cleanup) => {
-      if (error) {
-        reject(error)
-      } else {
-        resolve({ path, cleanup })
-      }
-    })
-  })
-}
-
-/**
- * @summary Disposer for tmpFileAsync, calls cleanup()
- * @function
- *
- * @returns {Disposer<{ path: String, cleanup: Function }>}
- *
- * @example
- * await Bluebird.using(tmpFileDisposer(), ({ path }) => {
- *   console.log(path);
- * })
- */
-const tmpFileDisposer = () => {
-  return Bluebird.resolve(tmpFileAsync())
-    .disposer(({ cleanup }) => {
-      cleanup()
-    })
-}
+const { tmpFileDisposer } = require('../../../shared/utils')
 
 const readFileAsync = promisify(fs.readFile)
 
@@ -99,7 +49,15 @@ exports.getWmicNetworkDrivesOutput = async () => {
   // We could also use wmic's "/output:" switch but it doesn't work when the filename
   // contains a space and the os temp dir may contain spaces ("D:\Windows Temp Files" for example).
   // So we just redirect to a file and read it afterwards as we know it will be ucs2 encoded.
-  return Bluebird.using(tmpFileDisposer(), async ({ path }) => {
+  const options = {
+
+    // Close the file once it's created
+    discardDescriptor: true,
+
+    // Wmic fails with "Invalid global switch" when the "/output:" switch filename contains a dash ("-")
+    prefix: 'tmp'
+  }
+  return Bluebird.using(tmpFileDisposer(options), async ({ path }) => {
     const command = [
       Path.join(process.env.SystemRoot, 'System32', 'Wbem', 'wmic'),
       'path',

lib/shared/permissions.js

@@ -14,17 +14,24 @@
  * limitations under the License.
  */
 
+/* eslint-disable lodash/prefer-lodash-method,quotes,no-magic-numbers,require-jsdoc */
+
 'use strict'
 
-const os = require('os')
-const nativeModule = require('./native-module')
 const Bluebird = require('bluebird')
 const childProcess = Bluebird.promisifyAll(require('child_process'))
-const sudoPrompt = Bluebird.promisifyAll(require('sudo-prompt'))
-const commandJoin = require('command-join')
+const fs = require('fs')
 const _ = require('lodash')
+const os = require('os')
+const sudoPrompt = Bluebird.promisifyAll(require('sudo-prompt'))
+const { promisify } = require('util')
+
 const errors = require('./errors')
 
+const { tmpFileDisposer } = require('./utils')
+
+const writeFileAsync = promisify(fs.writeFile)
+
 /**
  * @summary The user id of the UNIX "superuser"
  * @constant
@@ -90,68 +97,61 @@ exports.isElevatedUnixSync = () => {
   return (process.geteuid() === UNIX_SUPERUSER_USER_ID)
 }
 
-/**
- * @summary Get environment command prefix
- * @function
- * @private
- *
- * @param {Object} environment - environment map
- * @returns {String[]} command arguments
- *
- * @example
- * const commandPrefix = permissions.getEnvironmentCommandPrefix({
- *   FOO: 'bar',
- *   BAR: 'baz'
- * });
- *
- * childProcess.execSync(_.join(_.concat(commandPrefix, [ 'mycommand' ]), ' '));
- */
-exports.getEnvironmentCommandPrefix = (environment) => {
-  const isWindows = os.platform() === 'win32'
-
-  if (_.isEmpty(environment)) {
-    return []
-  }
-
-  const argv = _.flatMap(environment, (value, key) => {
-    if (_.isNil(value)) {
-      return []
-    }
-
-    if (isWindows) {
-      // Doing something like `set foo=bar &&` (notice
-      // the space before the first ampersand) would
-      // cause the environment variable's value to
-      // contain a trailing space.
-      // See https://superuser.com/a/57726
-      return [ 'set', `${key}=${value}&&` ]
-    }
-
-    return [ `${key}=${value}` ]
-  })
-
-  if (isWindows) {
-    // This is a trick to make the binary afterwards catch
-    // the environment variables set just previously.
-    return _.concat(argv, [ 'call' ])
-  }
-
-  return _.concat([ 'env' ], argv)
+const escapeSh = (value) => {
+  // Make sure it's a string
+  // Replace ' -> '\'' (closing quote, escaped quote, opening quote)
+  // Surround with quotes
+  return `'${String(value).replace(/'/g, "'\\''")}'`
 }
 
-/**
- * @summary Quote a string
- * @function
- * @private
- *
- * @param {String} string - input string
- * @returns {String} quoted string
- *
- * @example
- * const result = quote('foo');
- */
-const quoteString = (string) => {
-  return `"${string}"`
+const escapeParamCmd = (value) => {
+  // Make sure it's a string
+  // Escape " -> \"
+  // Surround with double quotes
+  return `"${String(value).replace(/"/g, '\\"')}"`
+}
+
+const setEnvVarSh = (value, name) => {
+  return `export ${name}=${escapeSh(value)}`
+}
+
+const setEnvVarCmd = (value, name) => {
+  return `set "${name}=${String(value)}"`
+}
+
+// Exported for tests
+exports.createLaunchScript = (command, argv, environment) => {
+  const isWindows = os.platform() === 'win32'
+  const lines = []
+  if (isWindows) {
+    // Switch to utf8
+    lines.push('chcp 65001')
+  }
+  const [ setEnvVarFn, escapeFn ] = isWindows ? [ setEnvVarCmd, escapeParamCmd ] : [ setEnvVarSh, escapeSh ]
+  lines.push(..._.map(environment, setEnvVarFn))
+  lines.push([ command, ...argv ].map(escapeFn).join(' '))
+  return lines.join(os.EOL)
+}
+
+const elevateScriptWindows = async (path) => {
+  // './nativeModule' imported here as it only exists on windows
+  // TODO: replace this with sudo-prompt once https://github.com/jorangreef/sudo-prompt/issues/96 is fixed
+  const nativeModule = require('./native-module')
+  const elevateAsync = promisify(nativeModule.load('elevator').elevate)
+
+  // '&' needs to be escaped here (but not when written to a .cmd file)
+  const cmd = [ 'cmd', '/c', escapeParamCmd(path).replace(/&/g, '^&') ]
+  const { cancelled } = await elevateAsync(cmd)
+  return { cancelled }
+}
+
+const elevateScriptUnix = async (path, name) => {
+  const cmd = [ 'sh', escapeSh(path) ].join(' ')
+  const [ , stderr ] = await sudoPrompt.execAsync(cmd, { name })
+  if (!_.isEmpty(stderr)) {
+    throw errors.createError({ title: stderr })
+  }
+  return { cancelled: false }
 }
 
 /**
@@ -178,73 +178,41 @@ const quoteString = (string) => {
  *   }
  * });
  */
-exports.elevateCommand = (command, options) => {
+exports.elevateCommand = async (command, options) => {
   const isWindows = os.platform() === 'win32'
-
-  const prefixedCommand = _.concat(
-    exports.getEnvironmentCommandPrefix(options.environment),
-    _.map(command, (string) => {
-      return isWindows ? quoteString(string) : string
-    })
-  )
-
-  if (isWindows) {
-    const elevator = Bluebird.promisifyAll(nativeModule.load('elevator'))
-    return elevator.elevateAsync([
-      'cmd.exe',
-      '/c',
-      quoteString(_.join(prefixedCommand, ' '))
-    ]).then((results) => {
-      return {
-        cancelled: results.cancelled
+  const launchScript = exports.createLaunchScript(command[0], command.slice(1), options.environment)
+  return Bluebird.using(tmpFileDisposer({ postfix: '.cmd' }), async ({ path }) => {
+    await writeFileAsync(path, launchScript)
+    if (isWindows) {
+      return elevateScriptWindows(path)
+    }
+    try {
+      return await elevateScriptUnix(path, options.applicationName)
+    } catch (error) {
+      // We're hardcoding internal error messages declared by `sudo-prompt`.
+      // There doesn't seem to be a better way to handle these errors, so
+      // for now, we should make sure we double check if the error messages
+      // have changed every time we upgrade `sudo-prompt`.
+      console.log('error', error)
+      if (_.includes(error.message, 'is not in the sudoers file')) {
+        throw errors.createUserError({
+          title: "Your user doesn't have enough privileges to proceed",
+          description: 'This application requires sudo privileges to be able to write to drives'
+        })
+      } else if (_.startsWith(error.message, 'Command failed:')) {
+        throw errors.createUserError({
+          title: 'The elevated process died unexpectedly',
+          description: `The process error code was ${error.code}`
+        })
+      } else if (error.message === 'User did not grant permission.') {
+        return { cancelled: true }
+      } else if (error.message === 'No polkit authentication agent found.') {
+        throw errors.createUserError({
+          title: 'No polkit authentication agent found',
+          description: 'Please install a polkit authentication agent for your desktop environment of choice to continue'
+        })
       }
-    })
-  }
-
-  return sudoPrompt.execAsync(commandJoin(prefixedCommand), {
-    name: options.applicationName
-  }).then((stdout, stderr) => {
-    if (!_.isEmpty(stderr)) {
-      throw errors.createError({
-        title: stderr
-      })
+      throw error
     }
-
-    return {
-      cancelled: false
-    }
-
-  // We're hardcoding internal error messages declared by `sudo-prompt`.
-  // There doesn't seem to be a better way to handle these errors, so
-  // for now, we should make sure we double check if the error messages
-  // have changed every time we upgrade `sudo-prompt`.
-  }).catch((error) => {
-    console.log('error', error.cause)
-    return _.includes(error.message, 'is not in the sudoers file')
-  }, () => {
-    throw errors.createUserError({
-      title: 'Your user doesn\'t have enough privileges to proceed',
-      description: 'This application requires sudo privileges to be able to write to drives'
-    })
-  }).catch((error) => {
-    return _.startsWith(error.message, 'Command failed:')
-  }, (error) => {
-    throw errors.createUserError({
-      title: 'The elevated process died unexpectedly',
-      description: `The process error code was ${error.code}`
-    })
-  }).catch({
-    message: 'User did not grant permission.'
-  }, () => {
-    return {
-      cancelled: true
-    }
-  }).catch({
-    message: 'No polkit authentication agent found.'
-  }, () => {
-    throw errors.createUserError({
-      title: 'No polkit authentication agent found',
-      description: 'Please install a polkit authentication agent for your desktop environment of choice to continue'
-    })
   })
 }

lib/shared/utils.js

@@ -19,6 +19,8 @@
 const _ = require('lodash')
 const Bluebird = require('bluebird')
 const request = Bluebird.promisifyAll(require('request'))
+const tmp = require('tmp')
+
 const errors = require('./errors')
 
 /**
@@ -168,3 +170,50 @@ exports.getConfig = (configUrl) => {
   return request.getAsync(configUrl, { json: true })
     .get('body')
 }
+
+/**
+ * @summary returns { path: String, cleanup: Function }
+ * @function
+ *
+ * @param {Object} options - options
+ *
+ * @returns {Promise<{ path: String, cleanup: Function }>}
+ *
+ * @example
+ * tmpFileAsync()
+ *   .then({ path, cleanup } => {
+ *     console.log(path)
+ *     cleanup()
+ *   });
+ */
+const tmpFileAsync = (options) => {
+  return new Promise((resolve, reject) => {
+    tmp.file(options, (error, path, _fd, cleanup) => {
+      if (error) {
+        reject(error)
+      } else {
+        resolve({ path, cleanup })
+      }
+    })
+  })
+}
+
+/**
+ * @summary Disposer for tmpFileAsync, calls cleanup()
+ * @function
+ *
+ * @param {Object} options - options
+ *
+ * @returns {Disposer<{ path: String, cleanup: Function }>}
+ *
+ * @example
+ * await Bluebird.using(tmpFileDisposer(), ({ path }) => {
+ *   console.log(path);
+ * })
+ */
+exports.tmpFileDisposer = (options) => {
+  return Bluebird.resolve(tmpFileAsync(options))
+    .disposer(({ cleanup }) => {
+      cleanup()
+    })
+}

npm-shrinkwrap.json

@@ -3131,11 +3131,6 @@
         "delayed-stream": "~1.0.0"
       }
     },
-    "command-join": {
-      "version": "2.0.0",
-      "resolved": "https://registry.npmjs.org/command-join/-/command-join-2.0.0.tgz",
-      "integrity": "sha1-Uui5hPSHLZUv8b3IuYOX0nxxRM8="
-    },
     "commander": {
       "version": "2.8.1",
       "resolved": "https://registry.npmjs.org/commander/-/commander-2.8.1.tgz",
@@ -13961,4 +13956,4 @@
       }
     }
   }
-}
+}

package.json

@@ -49,7 +49,6 @@
     "bluebird": "^3.5.3",
     "bootstrap-sass": "^3.3.6",
     "color": "^2.0.1",
-    "command-join": "^2.0.0",
     "d3": "^4.13.0",
     "debug": "^3.1.0",
     "electron-is-running-in-asar": "^1.0.0",

tests/shared/permissions.spec.js

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+/* eslint-disable quotes */
+
 'use strict'
 
 const m = require('mochainon')
@@ -21,7 +23,7 @@ const os = require('os')
 const permissions = require('../../lib/shared/permissions')
 
 describe('Shared: permissions', function () {
-  describe('.getEnvironmentCommandPrefix()', function () {
+  describe('.createLaunchScript()', function () {
     describe('given windows', function () {
       beforeEach(function () {
         this.osPlatformStub = m.sinon.stub(os, 'platform')
@@ -32,197 +34,62 @@ describe('Shared: permissions', function () {
         this.osPlatformStub.restore()
       })
 
-      it('should return an empty array if no environment', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix()).to.deep.equal([])
-      })
-
-      it('should return an empty array if the environment is an empty object', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({})).to.deep.equal([])
-      })
-
-      it('should create an environment command prefix out of one variable', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: 'bar'
-        })).to.deep.equal([
-          'set',
-          'FOO=bar&&',
-          'call'
-        ])
-      })
-
-      it('should create an environment command prefix out of many variables', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: 'bar',
-          BAR: 'baz',
-          BAZ: 'qux'
-        })).to.deep.equal([
-          'set',
-          'FOO=bar&&',
-          'set',
-          'BAR=baz&&',
-          'set',
-          'BAZ=qux&&',
-          'call'
-        ])
-      })
-
-      it('should ignore undefined and null variable values', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: null,
-          BAR: 'qux',
-          BAZ: undefined
-        })).to.deep.equal([
-          'set',
-          'BAR=qux&&',
-          'call'
-        ])
-      })
-
-      it('should stringify number values', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: 1,
-          BAR: 0,
-          BAZ: -1
-        })).to.deep.equal([
-          'set',
-          'FOO=1&&',
-          'set',
-          'BAR=0&&',
-          'set',
-          'BAZ=-1&&',
-          'call'
-        ])
+      it('should escape environment variables and arguments', function () {
+        m.chai.expect(
+          permissions.createLaunchScript(
+            "C:\\Users\\Alice & Bob's Laptop\\\"what\"\\balenaEtcher",
+            [
+              '"a Laser"',
+              'arg1',
+              "'&/ ^ \\",
+              '" $ % *'
+            ],
+            {
+              key: 'value',
+              key2: ' " \' ^ & = + $ % / \\',
+              key3: 8
+            }
+          )
+        ).to.equal(
+          `chcp 65001${os.EOL}` +
+          `set "key=value"${os.EOL}` +
+          `set "key2= " ' ^ & = + $ % / \\"${os.EOL}` +
+          `set "key3=8"${os.EOL}` +
+          `"C:\\Users\\Alice & Bob's Laptop\\\\"what\\"\\balenaEtcher" "\\"a Laser\\"" "arg1" "'&/ ^ \\" "\\" $ % *"`
+        )
       })
     })
 
-    describe('given linux', function () {
-      beforeEach(function () {
-        this.osPlatformStub = m.sinon.stub(os, 'platform')
-        this.osPlatformStub.returns('linux')
-      })
+    for (const platform of [ 'linux', 'darwin' ]) {
+      describe(`given ${platform}`, function () {
+        beforeEach(function () {
+          this.osPlatformStub = m.sinon.stub(os, 'platform')
+          this.osPlatformStub.returns(platform)
+        })
 
-      afterEach(function () {
-        this.osPlatformStub.restore()
-      })
+        afterEach(function () {
+          this.osPlatformStub.restore()
+        })
 
-      it('should return an empty array if no environment', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix()).to.deep.equal([])
+        it('should escape environment variables and arguments', function () {
+          m.chai.expect(
+            permissions.createLaunchScript(
+              "/home/Alice & Bob's Laptop/\"what\"/balenaEtcher",
+              [ 'arg1', "'&/ ^ \\", '" $ % *' ],
+              {
+                key: 'value',
+                key2: ' " \' ^ & = + $ % / \\',
+                key3: 8
+              }
+            )
+          ).to.equal(
+            `export key='value'${os.EOL}` +
+            `export key2=' " '\\'' ^ & = + $ % / \\'${os.EOL}` +
+            `export key3='8'${os.EOL}` +
+            `'/home/Alice & Bob'\\''s Laptop/"what"/balenaEtcher' 'arg1' ''\\''&/ ^ \\' '" $ % *'`
+          )
+        })
       })
-
-      it('should return an empty array if the environment is an empty object', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({})).to.deep.equal([])
-      })
-
-      it('should create an environment command prefix out of one variable', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: 'bar'
-        })).to.deep.equal([
-          'env',
-          'FOO=bar'
-        ])
-      })
-
-      it('should create an environment command prefix out of many variables', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: 'bar',
-          BAR: 'baz',
-          BAZ: 'qux'
-        })).to.deep.equal([
-          'env',
-          'FOO=bar',
-          'BAR=baz',
-          'BAZ=qux'
-        ])
-      })
-
-      it('should ignore undefined and null variable values', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: null,
-          BAR: 'qux',
-          BAZ: undefined
-        })).to.deep.equal([
-          'env',
-          'BAR=qux'
-        ])
-      })
-
-      it('should stringify number values', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: 1,
-          BAR: 0,
-          BAZ: -1
-        })).to.deep.equal([
-          'env',
-          'FOO=1',
-          'BAR=0',
-          'BAZ=-1'
-        ])
-      })
-    })
-
-    describe('given darwin', function () {
-      beforeEach(function () {
-        this.osPlatformStub = m.sinon.stub(os, 'platform')
-        this.osPlatformStub.returns('darwin')
-      })
-
-      afterEach(function () {
-        this.osPlatformStub.restore()
-      })
-
-      it('should return an empty array if no environment', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix()).to.deep.equal([])
-      })
-
-      it('should return an empty array if the environment is an empty object', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({})).to.deep.equal([])
-      })
-
-      it('should create an environment command prefix out of one variable', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: 'bar'
-        })).to.deep.equal([
-          'env',
-          'FOO=bar'
-        ])
-      })
-
-      it('should create an environment command prefix out of many variables', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: 'bar',
-          BAR: 'baz',
-          BAZ: 'qux'
-        })).to.deep.equal([
-          'env',
-          'FOO=bar',
-          'BAR=baz',
-          'BAZ=qux'
-        ])
-      })
-
-      it('should ignore undefined and null variable values', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: null,
-          BAR: 'qux',
-          BAZ: undefined
-        })).to.deep.equal([
-          'env',
-          'BAR=qux'
-        ])
-      })
-
-      it('should stringify number values', function () {
-        m.chai.expect(permissions.getEnvironmentCommandPrefix({
-          FOO: 1,
-          BAR: 0,
-          BAZ: -1
-        })).to.deep.equal([
-          'env',
-          'FOO=1',
-          'BAR=0',
-          'BAZ=-1'
-        ])
-      })
-    })
+    }
   })
 })