Modify from exec to execFile with _npmInstall (#303)

* Modify to arrow function

* Modify `var` to `const`

* Add test when codeDirectory contained characters to be escaped

* Modify from exec to execFile

- Arguments are escaped
- Modify to Arrow function

* Support Windows with execFile

* Fix command

* Remove unusable characters in Windows test

* Remove unusable characters in Windows test

Author: abetomo

lib/main.js

@@ -3,6 +3,7 @@
 var path = require('path')
 var aws = require('aws-sdk')
 var exec = require('child_process').exec
+var execFile = require('child_process').execFile
 var fs = require('fs-extra')
 var packageJson = require(path.join(__dirname, '..', 'package.json'))
 var minimatch = require('minimatch')
@@ -285,18 +286,55 @@ Lambda.prototype._rsync = function (program, src, dest, excludeNodeModules, call
   })
 }
 
-Lambda.prototype._npmInstall = function (program, codeDirectory, callback) {
-  const installOptions = [
-    `--prefix ${codeDirectory}`,
-    process.platform === 'win32' ? `--cwd ${codeDirectory}` : null
-  ].join(' ')
-  var command = program.dockerImage
-    ? 'docker run --rm -v ' + codeDirectory + ':/var/task ' + program.dockerImage + ' npm -s install --production'
-    : `npm -s install --production ${installOptions}`
-  exec(command, {
+Lambda.prototype._npmInstall = (program, codeDirectory, callback) => {
+  const dockerBaseOptions = [
+    'run', '--rm', '-v', `${codeDirectory}:/var/task`,
+    program.dockerImage,
+    'npm', '-s', 'install', '--production'
+  ]
+  const npmInstallBaseOptions = [
+    '-s',
+    'install',
+    '--production',
+    '--prefix', codeDirectory
+  ]
+
+  const params = (() => {
+    // reference: https://nodejs.org/api/child_process.html#child_process_spawning_bat_and_cmd_files_on_windows
+
+    // with docker
+    if (program.dockerImage) {
+      if (process.platform === 'win32') {
+        return {
+          command: 'cmd.exe',
+          options: ['/c', 'docker'].concat(dockerBaseOptions)
+        }
+      }
+      return {
+        command: 'docker',
+        options: dockerBaseOptions
+      }
+    }
+
+    // simple npm install
+    if (process.platform === 'win32') {
+      return {
+        command: 'cmd.exe',
+        options: ['/c', 'npm']
+          .concat(npmInstallBaseOptions)
+          .concat(['--cwd', codeDirectory])
+      }
+    }
+    return {
+      command: 'npm',
+      options: npmInstallBaseOptions
+    }
+  })()
+
+  execFile(params.command, params.options, {
     maxBuffer: maxBufferSize,
     env: process.env
-  }, function (err) {
+  }, (err) => {
     if (err) {
       return callback(err)
     }

test/main.js

@@ -321,14 +321,14 @@ describe('lib/main', function () {
     describe('_rsync', function () { rsyncTests('_rsync') })
   }
 
-  describe('_npmInstall', function () {
-    beforeEach(function (done) {
-      lambda._cleanDirectory(codeDirectory, function (err) {
+  describe('_npmInstall', () => {
+    beforeEach((done) => {
+      lambda._cleanDirectory(codeDirectory, (err) => {
         if (err) {
           return done(err)
         }
 
-        lambda._fileCopy(program, '.', codeDirectory, true, function (err) {
+        lambda._fileCopy(program, '.', codeDirectory, true, (err) => {
           if (err) {
             return done(err)
           }
@@ -340,9 +340,47 @@ describe('lib/main', function () {
     it('_npm adds node_modules', function (done) {
       _timeout({ this: this, sec: 30 }) // give it time to build the node modules
 
-      lambda._npmInstall(program, codeDirectory, function (err, result) {
+      lambda._npmInstall(program, codeDirectory, (err, result) => {
         assert.isNull(err)
-        var contents = fs.readdirSync(codeDirectory)
+        const contents = fs.readdirSync(codeDirectory)
+        assert.include(contents, 'node_modules')
+        done()
+      })
+    })
+  })
+
+  describe('_npmInstall (When codeDirectory contains characters to be escaped)', () => {
+    beforeEach((done) => {
+      // Since '\' can not be included in the file or directory name in Windows
+      const directoryName = process.platform === 'win32'
+        ? 'hoge fuga\' piyo'
+        : 'hoge "fuga\' \\piyo'
+      codeDirectory = path.join(os.tmpdir(), directoryName)
+      lambda._cleanDirectory(codeDirectory, (err) => {
+        if (err) {
+          return done(err)
+        }
+
+        lambda._fileCopy(program, '.', codeDirectory, true, (err) => {
+          if (err) {
+            return done(err)
+          }
+          done()
+        })
+      })
+    })
+
+    afterEach(() => {
+      fs.removeSync(codeDirectory)
+      codeDirectory = lambda._codeDirectory()
+    })
+
+    it('_npm adds node_modules', function (done) {
+      _timeout({ this: this, sec: 30 }) // give it time to build the node modules
+
+      lambda._npmInstall(program, codeDirectory, (err, result) => {
+        assert.isNull(err)
+        const contents = fs.readdirSync(codeDirectory)
         assert.include(contents, 'node_modules')
         done()
       })