Fix(which): match only executable files (#874)

termosa · nfischer · commit 8dae55ff4ac8 · 2018-07-18T15:33:08.000-07:00
On Unix, this only matches files with the exec bit set. On Windows, this only matches files which are readable (since Windows has different rules for execution). Fixes #657.
diff --git a/src/which.js b/src/which.js
@@ -13,13 +13,33 @@ 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;
+
+function isWindowsPlatform() {
+  return process.platform === 'win32';
+}
+
 // Cross-platform method for splitting environment `PATH` variables
 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
+    fs.accessSync(pathName, FILE_EXECUTABLE_MODE);
+  } 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()
+    && (isWindowsPlatform() || isExecutable(pathName));
 }
 
 //@
@@ -37,9 +57,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/resources/which/node b/test/resources/which/node
@@ -0,0 +1 @@
+text file, not an executable
diff --git 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,27 @@ 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 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)
+  t.truthy(fs.existsSync(matchingFile));
+
+  process.env.PATH = extraPath + path.delimiter + process.env.PATH;
+  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.is(resultForWhichA.indexOf(matchingFile), -1);
+
+  process.env.PATH = pathEnv;
 });
