From 06e9879066b9be4097f314e51974bf375307e71c Mon Sep 17 00:00:00 2001 From: Gleb Bahmutov Date: Wed, 26 Jun 2019 17:07:29 -0400 Subject: [PATCH 1/6] fix: trim and remove double quotes around CYPRESS_INSTALL_BINARY --- cli/lib/tasks/install.js | 11 ++++--- cli/lib/util.js | 45 ++++++++++++++++++++------- cli/test/lib/tasks/install_spec.js | 44 +++++++++++++++++++++++++- cli/test/lib/util_spec.js | 50 ++++++++++++++++++++++++++++++ 4 files changed, 133 insertions(+), 17 deletions(-) diff --git a/cli/lib/tasks/install.js b/cli/lib/tasks/install.js index 4f7b74922ada..992a35dfe5f7 100644 --- a/cli/lib/tasks/install.js +++ b/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') diff --git a/cli/lib/util.js b/cli/lib/util.js index 369f29c03175..731ce77acb0a 100644 --- a/cli/lib/util.js +++ b/cli/lib/util.js @@ -97,6 +97,18 @@ function printNodeOptions (log = debug) { } } +/** + * Removes double quote characters " + * from the start and end of the given string IF they are both present + */ +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 +187,8 @@ const util = { process.exit(1) }, + dequote, + titleize (...args) { // prepend first arg with space // and pad so that all messages line up @@ -255,31 +269,38 @@ 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 () { diff --git a/cli/test/lib/tasks/install_spec.js b/cli/test/lib/tasks/install_spec.js index 1a8574f9ba23..cba567ffe2d7 100644 --- a/cli/test/lib/tasks/install_spec.js +++ b/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 diff --git a/cli/test/lib/util_spec.js b/cli/test/lib/util_spec.js index 210655babd2e..29318f1b4485 100644 --- a/cli/test/lib/util_spec.js +++ b/cli/test/lib/util_spec.js @@ -356,6 +356,20 @@ 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') + }) + }) + describe('.getEnv', () => { it('reads from package.json config', () => { process.env.npm_package_config_CYPRESS_FOO = 'bar' @@ -379,5 +393,41 @@ 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\'') + }) + }) }) }) From fd52be385157aca04d3373dfc960b88330ce497c Mon Sep 17 00:00:00 2001 From: Gleb Bahmutov Date: Wed, 26 Jun 2019 17:07:42 -0400 Subject: [PATCH 2/6] linting --- cli/lib/util.js | 4 +++- cli/test/lib/util_spec.js | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cli/lib/util.js b/cli/lib/util.js index 731ce77acb0a..d0d0dd15c2cd 100644 --- a/cli/lib/util.js +++ b/cli/lib/util.js @@ -103,9 +103,10 @@ function printNodeOptions (log = debug) { */ 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] === '"') { + if (str.length > 1 && str[0] === '"' && str[str.length - 1] === '"') { return str.substr(1, str.length - 2) } + return str } @@ -277,6 +278,7 @@ const util = { const packageConfigVar = process.env[`npm_package_config_${varName}`] let result + if (envVar) { debug(`Using ${varName} from environment variable`) diff --git a/cli/test/lib/util_spec.js b/cli/test/lib/util_spec.js index 29318f1b4485..763e85149f67 100644 --- a/cli/test/lib/util_spec.js +++ b/cli/test/lib/util_spec.js @@ -362,7 +362,7 @@ describe('util', () => { }) it('keeps single quotes', () => { - expect(util.dequote("'foo'")).to.equal("'foo'") + expect(util.dequote('\'foo\'')).to.equal('\'foo\'') }) it('keeps unbalanced double quotes', () => { From c5fb7cc3ffabeed5315386910534c2cb0baefa6e Mon Sep 17 00:00:00 2001 From: Gleb Bahmutov Date: Wed, 26 Jun 2019 17:08:35 -0400 Subject: [PATCH 3/6] add one more unit test for dequote --- cli/test/lib/util_spec.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cli/test/lib/util_spec.js b/cli/test/lib/util_spec.js index 763e85149f67..56992e55fc7b 100644 --- a/cli/test/lib/util_spec.js +++ b/cli/test/lib/util_spec.js @@ -368,6 +368,10 @@ describe('util', () => { 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') + }) }) describe('.getEnv', () => { From ce50d4e8bcacb01bf43c9fb31fccdf11cf7d0112 Mon Sep 17 00:00:00 2001 From: Gleb Bahmutov Date: Thu, 27 Jun 2019 08:57:21 -0400 Subject: [PATCH 4/6] add one more unit test --- cli/test/lib/util_spec.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cli/test/lib/util_spec.js b/cli/test/lib/util_spec.js index 56992e55fc7b..88a67eccb052 100644 --- a/cli/test/lib/util_spec.js +++ b/cli/test/lib/util_spec.js @@ -432,6 +432,11 @@ describe('util', () => { 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 ') + }) }) }) }) From 5455d440b959590aa359aa1077c6a8e4f5363c5c Mon Sep 17 00:00:00 2001 From: Gleb Bahmutov Date: Thu, 27 Jun 2019 08:59:53 -0400 Subject: [PATCH 5/6] fix jsdoc --- cli/lib/util.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/cli/lib/util.js b/cli/lib/util.js index d0d0dd15c2cd..f7d2ab7c59d9 100644 --- a/cli/lib/util.js +++ b/cli/lib/util.js @@ -98,9 +98,17 @@ function printNodeOptions (log = debug) { } /** - * Removes double quote characters " - * from the start and end of the given string IF they are both present - */ + * 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] === '"') { From abfe6efc636171f803983a6d213bc96f3d15f677 Mon Sep 17 00:00:00 2001 From: Gleb Bahmutov Date: Thu, 27 Jun 2019 09:25:38 -0400 Subject: [PATCH 6/6] a few more edge unit tests --- cli/test/lib/util_spec.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cli/test/lib/util_spec.js b/cli/test/lib/util_spec.js index 88a67eccb052..8f46dc004832 100644 --- a/cli/test/lib/util_spec.js +++ b/cli/test/lib/util_spec.js @@ -372,6 +372,14 @@ describe('util', () => { 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', () => {