From 79b8feb78d9e40b3a134eb3253501d95a0cb36af Mon Sep 17 00:00:00 2001 From: Stanislav Termosa Date: Thu, 12 Jul 2018 21:32:17 +0300 Subject: [PATCH 1/4] fix(which): match only executable files (#657) --- src/which.js | 11 ++++++++++- test/resources/which/node | 1 + test/which.js | 23 ++++++++++++++++++++++- 3 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 test/resources/which/node diff --git a/src/which.js b/src/which.js index 0e5433c0..ed02ab8d 100644 --- a/src/which.js +++ b/src/which.js @@ -18,8 +18,17 @@ function splitPath(p) { return p ? p.split(path.delimiter) : []; } +function isExecutable(pathName) { + try { + fs.accessSync(pathName, fs.constants.X_OK); + } catch (err) { + return false; + } + return true; +} + function checkPath(pathName) { - return fs.existsSync(pathName) && !common.statFollowLinks(pathName).isDirectory(); + return fs.existsSync(pathName) && !common.statFollowLinks(pathName).isDirectory() && isExecutable(pathName); } //@ diff --git a/test/resources/which/node b/test/resources/which/node new file mode 100644 index 00000000..0b917ba3 --- /dev/null +++ b/test/resources/which/node @@ -0,0 +1 @@ +text file, not an executable diff --git a/test/which.js b/test/which.js index 530fcd2c..3a7ab977 100644 --- a/test/which.js +++ b/test/which.js @@ -1,4 +1,5 @@ import fs from 'fs'; +import path from 'path'; import test from 'ava'; @@ -69,5 +70,25 @@ test('Searching with -a flag returns an array with first item equals to the regu t.falsy(shell.error()); t.truthy(resultForWhich); t.truthy(resultForWhichA); - t.is(resultForWhich.toString(), resultForWhichA[0].toString()); + t.is(resultForWhich.toString(), resultForWhichA[0]); +}); + +test('None executable files does not appear in the result list', t => { + const commandName = 'node'; // Should be an existing command + const pathEnv = process.env.path || process.env.Path || process.env.PATH; + const extraPath = path.resolve(__dirname, 'resources', 'which'); + const matchingFile = path.resolve(extraPath, commandName); + + // make sure that file is exists (will throw error otherwise) + fs.statSync(matchingFile); + + process.env.path = process.env.Path = process.env.PATH = extraPath + path.delimiter + pathEnv; + const resultForWhich = shell.which(commandName); + const resultForWhichA = shell.which('-a', commandName); + t.falsy(shell.error()); + t.truthy(resultForWhich); + t.truthy(resultForWhichA); + t.truthy(resultForWhichA.length); + t.not(resultForWhich.toString(), matchingFile); + t.not(resultForWhichA[0], matchingFile); }); From 644e63208b1678a8997b1b0abf10b7c9ec4c62c8 Mon Sep 17 00:00:00 2001 From: Stanislav Termosa Date: Thu, 12 Jul 2018 22:45:05 +0300 Subject: [PATCH 2/4] fix(which): replace constant to support earlier versions of NodeJS (#657) --- src/which.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/which.js b/src/which.js index ed02ab8d..db326486 100644 --- a/src/which.js +++ b/src/which.js @@ -13,6 +13,9 @@ common.register('which', _which, { // set on Windows. var XP_DEFAULT_PATHEXT = '.com;.exe;.bat;.cmd;.vbs;.vbe;.js;.jse;.wsf;.wsh'; +// For earlier versions of NodeJS that doesn't have a list of constants (< v6) +var FILE_EXECUTABLE_MODE = 1; + // Cross-platform method for splitting environment `PATH` variables function splitPath(p) { return p ? p.split(path.delimiter) : []; @@ -20,7 +23,7 @@ function splitPath(p) { function isExecutable(pathName) { try { - fs.accessSync(pathName, fs.constants.X_OK); + fs.accessSync(pathName, FILE_EXECUTABLE_MODE); } catch (err) { return false; } From 9157b5396aacd2766ee28b4478b28f173777df8b Mon Sep 17 00:00:00 2001 From: Stanislav Termosa Date: Sat, 14 Jul 2018 13:54:52 +0300 Subject: [PATCH 3/4] fix(which): address @nfischer comments (#657) --- src/which.js | 13 +++++++++---- test/which.js | 9 ++++++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/which.js b/src/which.js index db326486..1591ebeb 100644 --- a/src/which.js +++ b/src/which.js @@ -16,6 +16,10 @@ var XP_DEFAULT_PATHEXT = '.com;.exe;.bat;.cmd;.vbs;.vbe;.js;.jse;.wsf;.wsh'; // For earlier versions of NodeJS that doesn't have a list of constants (< v6) var FILE_EXECUTABLE_MODE = 1; +function isWindowsPlatform() { + return process.platform === 'win32'; +} + // Cross-platform method for splitting environment `PATH` variables function splitPath(p) { return p ? p.split(path.delimiter) : []; @@ -23,6 +27,7 @@ function splitPath(p) { function isExecutable(pathName) { try { + // TODO(node-support): replace with fs.constants.X_OK once remove support for node < v6 fs.accessSync(pathName, FILE_EXECUTABLE_MODE); } catch (err) { return false; @@ -31,7 +36,8 @@ function isExecutable(pathName) { } function checkPath(pathName) { - return fs.existsSync(pathName) && !common.statFollowLinks(pathName).isDirectory() && isExecutable(pathName); + return fs.existsSync(pathName) && !common.statFollowLinks(pathName).isDirectory() + && (isWindowsPlatform() || isExecutable(pathName)); } //@ @@ -49,9 +55,8 @@ function checkPath(pathName) { function _which(options, cmd) { if (!cmd) common.error('must specify command'); - var isWindows = process.platform === 'win32'; - var pathEnv = process.env.path || process.env.Path || process.env.PATH; - var pathArray = splitPath(pathEnv); + var isWindows = isWindowsPlatform(); + var pathArray = splitPath(process.env.PATH); var queryMatches = []; diff --git a/test/which.js b/test/which.js index 3a7ab977..1f3fbe7d 100644 --- a/test/which.js +++ b/test/which.js @@ -75,14 +75,14 @@ test('Searching with -a flag returns an array with first item equals to the regu test('None executable files does not appear in the result list', t => { const commandName = 'node'; // Should be an existing command - const pathEnv = process.env.path || process.env.Path || process.env.PATH; const extraPath = path.resolve(__dirname, 'resources', 'which'); const matchingFile = path.resolve(extraPath, commandName); + const pathEnv = process.env.PATH; // make sure that file is exists (will throw error otherwise) - fs.statSync(matchingFile); + t.truthy(fs.existsSync(matchingFile)); - process.env.path = process.env.Path = process.env.PATH = extraPath + path.delimiter + pathEnv; + process.env.PATH = extraPath + path.delimiter + process.env.PATH; const resultForWhich = shell.which(commandName); const resultForWhichA = shell.which('-a', commandName); t.falsy(shell.error()); @@ -90,5 +90,8 @@ test('None executable files does not appear in the result list', t => { t.truthy(resultForWhichA); t.truthy(resultForWhichA.length); t.not(resultForWhich.toString(), matchingFile); + // TODO(node-support): this can be used starting from node@6: t.falsy(resultForWhichA.includes(matchingFile)) t.not(resultForWhichA[0], matchingFile); + + process.env.PATH = pathEnv; }); From ce705425948bae09095e1ee7eb007389dfc3ab6b Mon Sep 17 00:00:00 2001 From: Stanislav Termosa Date: Sun, 15 Jul 2018 12:19:20 +0300 Subject: [PATCH 4/4] fix(which): use indexOf (#657) --- src/which.js | 2 ++ test/which.js | 3 +-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/which.js b/src/which.js index 1591ebeb..cc497384 100644 --- a/src/which.js +++ b/src/which.js @@ -25,6 +25,8 @@ function splitPath(p) { return p ? p.split(path.delimiter) : []; } +// Tests are running all cases for this func but it stays uncovered by codecov due to unknown reason +/* istanbul ignore next */ function isExecutable(pathName) { try { // TODO(node-support): replace with fs.constants.X_OK once remove support for node < v6 diff --git a/test/which.js b/test/which.js index 1f3fbe7d..f81d9e0e 100644 --- a/test/which.js +++ b/test/which.js @@ -90,8 +90,7 @@ test('None executable files does not appear in the result list', t => { t.truthy(resultForWhichA); t.truthy(resultForWhichA.length); t.not(resultForWhich.toString(), matchingFile); - // TODO(node-support): this can be used starting from node@6: t.falsy(resultForWhichA.includes(matchingFile)) - t.not(resultForWhichA[0], matchingFile); + t.is(resultForWhichA.indexOf(matchingFile), -1); process.env.PATH = pathEnv; });