Clean env variable CYPRESS_INSTALL_BINARY before checking

cli/lib/tasks/install.js

@@ -18,9 +18,9 @@ const { throwFormErrorText, errors } = require('../errors')
 
 const alreadyInstalledMsg = () => {
   if (!util.isPostInstall()) {
-    logger.log(stripIndent`    
+    logger.log(stripIndent`
       Skipping installation:
-  
+
         Pass the ${chalk.yellow('--force')} option if you'd like to reinstall anyway.
     `)
   }
@@ -153,9 +153,12 @@ const start = (options = {}) => {
   // let this environment variable reset the binary version we need
   if (util.getEnv('CYPRESS_INSTALL_BINARY')) {
 
-    const envVarVersion = util.getEnv('CYPRESS_INSTALL_BINARY')
+    // because passed file paths are often double quoted
+    // and might have extra whitespace around, be robust and trim the string
+    const trimAndRemoveDoubleQuotes = true
+    const envVarVersion = util.getEnv('CYPRESS_INSTALL_BINARY', trimAndRemoveDoubleQuotes)
 
-    debug('using environment variable CYPRESS_INSTALL_BINARY %s', envVarVersion)
+    debug('using environment variable CYPRESS_INSTALL_BINARY "%s"', envVarVersion)
 
     if (envVarVersion === '0') {
       debug('environment variable CYPRESS_INSTALL_BINARY = 0, skipping install')

cli/lib/util.js

@@ -97,6 +97,27 @@ function printNodeOptions (log = debug) {
   }
 }
 
+/**
+ * Removes double quote characters
+ * from the start and end of the given string IF they are both present
+ *
+ * @example
+  ```
+  dequote('"foo"')
+  // returns string 'foo'
+  dequote('foo')
+  // returns string 'foo'
+  ```
+ */
+const dequote = (str) => {
+  la(is.string(str), 'expected a string to remove double quotes', str)
+  if (str.length > 1 && str[0] === '"' && str[str.length - 1] === '"') {
+    return str.substr(1, str.length - 2)
+  }
+
+  return str
+}
+
 const util = {
   normalizeModuleOptions,
 
@@ -175,6 +196,8 @@ const util = {
     process.exit(1)
   },
 
+  dequote,
+
   titleize (...args) {
     // prepend first arg with space
     // and pad so that all messages line up
@@ -262,31 +285,39 @@ const util = {
     return path.join(process.cwd(), '..', '..', filename)
   },
 
-  getEnv (varName) {
+  getEnv (varName, trim) {
+    la(is.unemptyString(varName), 'expected environment variable name, not', varName)
+
     const envVar = process.env[varName]
     const configVar = process.env[`npm_config_${varName}`]
     const packageConfigVar = process.env[`npm_package_config_${varName}`]
 
+    let result
+
     if (envVar) {
       debug(`Using ${varName} from environment variable`)
 
-      return envVar
-    }
-
-    if (configVar) {
+      result = envVar
+    } else if (configVar) {
       debug(`Using ${varName} from npm config`)
 
-      return configVar
-    }
-
-    if (packageConfigVar) {
+      result = configVar
+    } else if (packageConfigVar) {
       debug(`Using ${varName} from package.json config`)
 
-      return packageConfigVar
+      result = packageConfigVar
     }
 
-    return undefined
-
+    // environment variables are often set double quotes to escape characters
+    // and on Windows it can lead to weird things: for example
+    //  set FOO="C:\foo.txt" && node -e "console.log('>>>%s<<<', process.env.FOO)"
+    // will print
+    //    >>>"C:\foo.txt" <<<
+    // see https://github.com/cypress-io/cypress/issues/4506#issuecomment-506029942
+    // so for sanity sake we should first trim whitespace characters and remove
+    // double quotes around environment strings if the caller is expected to
+    // use this environment string as a file path
+    return trim ? dequote(_.trim(result)) : result
   },
 
   getCacheDir () {

cli/test/lib/tasks/install_spec.js

@@ -99,7 +99,49 @@ describe('/lib/tasks/install', function () {
         })
       })
 
-      it('can install local binary zip file without download', function () {
+      it('trims environment variable before installing', function () {
+        // note how the version has extra spaces around it on purpose
+        const filename = '/tmp/local/file.zip'
+        const version = ` ${filename}   `
+
+        process.env.CYPRESS_INSTALL_BINARY = version
+        // internally, the variable should be trimmed and just filename checked
+        sinon.stub(fs, 'pathExistsAsync').withArgs(filename).resolves(true)
+
+        const installDir = state.getVersionDir()
+
+        return install.start()
+        .then(() => {
+          expect(unzip.start).to.be.calledWithMatch({
+            zipFilePath: filename,
+            installDir,
+          })
+        })
+      })
+
+      it('removes double quotes around the environment variable before installing', function () {
+        // note how the version has extra spaces around it on purpose
+        // and there are double quotes
+        const filename = '/tmp/local/file.zip'
+        const version = ` "${filename}"   `
+
+        process.env.CYPRESS_INSTALL_BINARY = version
+        // internally, the variable should be trimmed, double quotes removed
+        //  and just filename checked against the file system
+        sinon.stub(fs, 'pathExistsAsync').withArgs(filename).resolves(true)
+
+        const installDir = state.getVersionDir()
+
+        return install.start()
+        .then(() => {
+          expect(unzip.start).to.be.calledWithMatch({
+            zipFilePath: filename,
+            installDir,
+          })
+        })
+      })
+
+      it('can install local binary zip file without download from absolute path', function () {
         const version = '/tmp/local/file.zip'
 
         process.env.CYPRESS_INSTALL_BINARY = version

cli/test/lib/util_spec.js

@@ -380,6 +380,32 @@ describe('util', () => {
     })
   })
 
+  describe('dequote', () => {
+    it('removes double quotes', () => {
+      expect(util.dequote('"foo"')).to.equal('foo')
+    })
+
+    it('keeps single quotes', () => {
+      expect(util.dequote('\'foo\'')).to.equal('\'foo\'')
+    })
+
+    it('keeps unbalanced double quotes', () => {
+      expect(util.dequote('"foo')).to.equal('"foo')
+    })
+
+    it('keeps inner double quotes', () => {
+      expect(util.dequote('a"b"c')).to.equal('a"b"c')
+    })
+
+    it('passes empty strings', () => {
+      expect(util.dequote('')).to.equal('')
+    })
+
+    it('keeps single double quote character', () => {
+      expect(util.dequote('"')).to.equal('"')
+    })
+  })
+
   describe('.getEnv', () => {
     it('reads from package.json config', () => {
       process.env.npm_package_config_CYPRESS_FOO = 'bar'
@@ -403,5 +429,46 @@ describe('util', () => {
       process.env.npm_config_CYPRESS_FOO = 'bloop'
       expect(util.getEnv('CYPRESS_FOO')).to.eql('bloop')
     })
+    it('throws on non-string name', () => {
+      expect(() => {
+        util.getEnv()
+      }).to.throw()
+
+      expect(() => {
+        util.getEnv(42)
+      }).to.throw()
+    })
+
+    context('with trim = true', () => {
+      it('trims returned string', () => {
+        process.env.FOO = '  bar  '
+        expect(util.getEnv('FOO', true)).to.equal('bar')
+      })
+
+      it('removes quotes from the returned string', () => {
+        process.env.FOO = '  "bar"  '
+        expect(util.getEnv('FOO', true)).to.equal('bar')
+      })
+
+      it('removes only single level of double quotes', () => {
+        process.env.FOO = '  ""bar""  '
+        expect(util.getEnv('FOO', true)).to.equal('"bar"')
+      })
+
+      it('keeps unbalanced double quote', () => {
+        process.env.FOO = '  "bar  '
+        expect(util.getEnv('FOO', true)).to.equal('"bar')
+      })
+
+      it('trims but does not remove single quotes', () => {
+        process.env.FOO = '  \'bar\'  '
+        expect(util.getEnv('FOO', true)).to.equal('\'bar\'')
+      })
+
+      it('keeps whitespace inside removed quotes', () => {
+        process.env.FOO = '"foo.txt "'
+        expect(util.getEnv('FOO', true)).to.equal('foo.txt ')
+      })
+    })
   })
 })