Provide explicit $PATH value to which (#1142)

Update the implementation of `resolveExecutable` to accept the
environment variables so that they can explicitly be provided to
which [1]. All internal code and tests have been updated accordingly,
no external changes.

This is in an effort to fix a problem where environment variables aren't
always passed on correctly to subprocesses. The problem is described in
[2], though if Shescape is used in a forked process on Windows it would
face the same problem. (As a problem found through the test runner, use
of which [1] in tests has been updated similarly.

As a result of this change, the integration and e2e test on Windows are
quite a bit slower (because now they actually look up the executable).
Hence, both of these suites have been updated to use separated test
files in order to better leverage AVA's concurrency - speeding up
individual files and avoiding timeouts as well as running tests quicker
if cores are available.

This change also allowed for including executables without a file
extension in the integration and e2e tests, further improving coverage.
(This replaced the `.EXE` coverage as extensionless executables are
resolved to `.EXE`, thus reducing test count without reducing coverage.)

As a side note, it was funny to see that `getShellName` name unit tests
for both Unix and Windows already generated and provided environment
variables (:

--
1. https://www.npmjs.com/package/which
2. avajs/ava#3014

CHANGELOG.md

@@ -7,6 +7,7 @@ Versioning].
 
 ## [Unreleased]
 
+- Fix potential silent executable lookup failure for Windows. ([#1142])
 - Support more valid `shell` values for Windows. ([#1137])
 
 ## [1.7.3] - 2023-08-07
@@ -292,6 +293,7 @@ Versioning].
 [#1083]: https://github.com/ericcornelissen/shescape/pull/1083
 [#1094]: https://github.com/ericcornelissen/shescape/pull/1094
 [#1137]: https://github.com/ericcornelissen/shescape/pull/1137
+[#1142]: https://github.com/ericcornelissen/shescape/pull/1142
 [552e8ea]: https://github.com/ericcornelissen/shescape/commit/552e8eab56861720b1d4e5474fb65741643358f9
 [keep a changelog]: https://keepachangelog.com/en/1.0.0/
 [semantic versioning]: https://semver.org/spec/v2.0.0.html

index.js

@@ -53,7 +53,7 @@ function getPlatformHelpers() {
 export function escape(arg, options = {}) {
   const helpers = getPlatformHelpers();
   const { flagProtection, interpolation, shellName } = parseOptions(
-    { options, process },
+    { env: process.env, options },
     helpers,
   );
   const argAsString = checkedToString(arg);
@@ -129,7 +129,7 @@ export function escapeAll(args, options = {}) {
 export function quote(arg, options = {}) {
   const helpers = getPlatformHelpers();
   const { flagProtection, shellName } = parseOptions(
-    { options, process },
+    { env: process.env, options },
     helpers,
   );
   const argAsString = checkedToString(arg);

src/executables.js

@@ -12,16 +12,20 @@
  * - Follows symbolic links.
  *
  * @param {object} args The arguments for this function.
+ * @param {object} args.env The environment variables.
  * @param {string} args.executable A string representation of the executable.
  * @param {object} deps The dependencies for this function.
  * @param {Function} deps.exists A function to check if a file exists.
  * @param {Function} deps.readlink A function to resolve (sym)links.
  * @param {Function} deps.which A function to perform a `which(1)`-like lookup.
  * @returns {string} The full path to the binary of the executable.
  */
-export function resolveExecutable({ executable }, { exists, readlink, which }) {
+export function resolveExecutable(
+  { env, executable },
+  { exists, readlink, which },
+) {
   try {
-    executable = which(executable);
+    executable = which(executable, { path: env.PATH || env.Path });
   } catch (_) {
     // For backwards compatibility return the executable even if its location
     // cannot be obtained

src/options.js

@@ -10,25 +10,24 @@ import { isString } from "./reflection.js";
  * Parses options provided to shescape.
  *
  * @param {object} args The arguments for this function.
+ * @param {object} args.env The environment variables.
  * @param {object} args.options The options for escaping.
  * @param {boolean} [args.options.flagProtection] Is flag protection enabled.
  * @param {boolean} [args.options.interpolation] Is interpolation enabled.
  * @param {boolean | string} [args.options.shell] The shell to escape for.
- * @param {object} args.process The `process` values.
- * @param {object} args.process.env The environment variables.
  * @param {object} deps The dependencies for this function.
  * @param {Function} deps.getDefaultShell Function to get the default shell.
  * @param {Function} deps.getShellName Function to get the name of a shell.
  * @returns {object} The parsed arguments.
  */
 export function parseOptions(
-  { options: { flagProtection, interpolation, shell }, process: { env } },
+  { env, options: { flagProtection, interpolation, shell } },
   { getDefaultShell, getShellName },
 ) {
   flagProtection = flagProtection ? true : false;
   interpolation = interpolation ? true : false;
   shell = isString(shell) ? shell : getDefaultShell({ env });
 
-  const shellName = getShellName({ shell }, { resolveExecutable });
+  const shellName = getShellName({ env, shell }, { resolveExecutable });
   return { flagProtection, interpolation, shellName };
 }

src/unix.js

@@ -121,14 +121,15 @@ export function getFlagProtectionFunction(shellName) {
  * Determines the name of the shell identified by a file path or file name.
  *
  * @param {object} args The arguments for this function.
+ * @param {object} args.env The environment variables.
  * @param {string} args.shell The name or path of the shell.
  * @param {object} deps The dependencies for this function.
  * @param {Function} deps.resolveExecutable Resolve the path to an executable.
  * @returns {string} The shell name.
  */
-export function getShellName({ shell }, { resolveExecutable }) {
+export function getShellName({ env, shell }, { resolveExecutable }) {
   shell = resolveExecutable(
-    { executable: shell },
+    { env, executable: shell },
     { exists: fs.existsSync, readlink: fs.readlinkSync, which: which.sync },
   );
 

src/win.js

@@ -98,14 +98,15 @@ export function getFlagProtectionFunction(shellName) {
  * Determines the name of the shell identified by a file path or file name.
  *
  * @param {object} args The arguments for this function.
+ * @param {object} args.env The environment variables.
  * @param {string} args.shell The name or path of the shell.
  * @param {object} deps The dependencies for this function.
  * @param {Function} deps.resolveExecutable Resolve the path to an executable.
  * @returns {string} The shell name.
  */
-export function getShellName({ shell }, { resolveExecutable }) {
+export function getShellName({ env, shell }, { resolveExecutable }) {
   shell = resolveExecutable(
-    { executable: shell },
+    { env, executable: shell },
     { exists: fs.existsSync, readlink: fs.readlinkSync, which: which.sync },
   );
 

test/_constants.cjs

@@ -60,11 +60,13 @@ module.exports.shellsUnix = [
 
 /* Windows related constants */
 module.exports.binCmd = "cmd.exe";
+module.exports.binCmdNoExt = "cmd";
 module.exports.binPowerShell = "powershell.exe";
+module.exports.binPowerShellNoExt = "powershell";
 
 module.exports.shellsWindows = [
   module.exports.binCmd,
-  "cmd.EXE",
+  module.exports.binCmdNoExt,
   module.exports.binPowerShell,
-  "powershell.EXE",
+  module.exports.binPowerShellNoExt,
 ];

test/e2e/_.js

@@ -3,8 +3,8 @@
  * @license MIT
  */
 
-import { injectionStrings } from "../../testing.js";
 import * as constants from "../_constants.cjs";
+import * as common from "./_common.js";
 import * as macros from "./_macros.js";
 
-export { constants, macros, injectionStrings };
+export { common, constants, macros };

test/e2e/_common.js

@@ -0,0 +1,53 @@
+/**
+ * @overview Provides common utilities for end-to-end tests.
+ * @license MIT
+ */
+
+import process from "node:process";
+
+import test from "ava";
+import isCI from "is-ci";
+import which from "which";
+
+import { injectionStrings } from "../../testing.js";
+import * as constants from "../_constants.cjs";
+
+/**
+ * Get a list of strings to use as arguments in end-to-end tests.
+ *
+ * @returns {string[]} A list of test arguments.
+ */
+export function getTestArgs() {
+  return ["harmless", ...injectionStrings];
+}
+
+/**
+ * Get the AVA test function to use for the given shell.
+ *
+ * @param {string} shell The shell to run a test for.
+ * @returns {Function} An AVA `test` function.
+ */
+export function getTestFn(shell) {
+  try {
+    if (!isCI && typeof shell === "string") {
+      which.sync(shell, { path: process.env.PATH || process.env.Path });
+    }
+
+    return test;
+  } catch (_) {
+    return test.skip;
+  }
+}
+
+/**
+ * Get a list of `shell` option values to use in end-to-end tests.
+ *
+ * @returns {(boolean | string)[]} A list of `shell` option values.
+ */
+export function getTestShells() {
+  const systemShells = constants.isWindows
+    ? constants.shellsWindows
+    : constants.shellsUnix;
+
+  return [false, true, ...systemShells];
+}

test/e2e/exec-file.test.js

@@ -0,0 +1,15 @@
+/**
+ * @overview Contains end-to-end tests of using Shescape with the child_process
+ * package's `execFile` (and `execFileSync`) functions.
+ * @license MIT
+ */
+
+import { common, macros } from "./_.js";
+
+for (const shell of common.getTestShells()) {
+  const test = common.getTestFn(shell);
+  for (const arg of common.getTestArgs()) {
+    test(macros.execFile, { arg, shell });
+    test(macros.execFileSync, { arg, shell });
+  }
+}

test/e2e/exec.test.js

@@ -0,0 +1,15 @@
+/**
+ * @overview Contains end-to-end tests of using Shescape with the child_process
+ * package's `exec` (and `execSync`) functions.
+ * @license MIT
+ */
+
+import { common, macros } from "./_.js";
+
+for (const shell of common.getTestShells()) {
+  const test = common.getTestFn(shell);
+  for (const arg of common.getTestArgs()) {
+    test(macros.exec, { arg, shell });
+    test(macros.execSync, { arg, shell });
+  }
+}

test/e2e/fork.test.js

@@ -0,0 +1,12 @@
+/**
+ * @overview Contains end-to-end tests of using Shescape with the child_process
+ * package's `fork` functions.
+ * @license MIT
+ */
+
+import { common, macros } from "./_.js";
+
+for (const arg of common.getTestArgs()) {
+  const test = common.getTestFn(null);
+  test(macros.fork, { arg });
+}

test/e2e/spawn.test.js

@@ -0,0 +1,15 @@
+/**
+ * @overview Contains end-to-end tests of using Shescape with the child_process
+ * package's `spawn` (and `spawnSync`) functions.
+ * @license MIT
+ */
+
+import { common, macros } from "./_.js";
+
+for (const shell of common.getTestShells()) {
+  const test = common.getTestFn(shell);
+  for (const arg of common.getTestArgs()) {
+    test(macros.spawn, { arg, shell });
+    test(macros.spawnSync, { arg, shell });
+  }
+}

test/integration/_generators.js

@@ -5,28 +5,16 @@
 
 import * as fixturesUnix from "../fixtures/unix.js";
 import * as fixturesWindows from "../fixtures/win.js";
-import common from "../_constants.cjs";
 
-/**
- * Returns the shells officially supported by Shescape for the current platform.
- *
- * @yields {string} Supported shells for the current platform.
- */
-export function* platformShells() {
-  if (common.isWindows) {
-    yield* common.shellsWindows;
-  } else {
-    yield* common.shellsUnix;
-  }
-}
+import { constants } from "./_.js";
 
 /**
  * Returns the test fixtures for the current platform.
  *
  * @returns {object} All test fixtures for the current platform.
  */
 function getPlatformFixtures() {
-  if (common.isWindows) {
+  if (constants.isWindows) {
     return fixturesWindows;
   } else {
     return fixturesUnix;
@@ -40,13 +28,16 @@ function getPlatformFixtures() {
  * @returns {object} All test fixtures for `shell`.
  */
 function getShellFixtures(shell) {
-  shell = shell.toLowerCase();
+  let shellName = shell.toLowerCase();
+  if (constants.isWindows) {
+    shellName = shellName.endsWith(".exe") ? shellName : `${shellName}.exe`;
+  }
 
   const fixtures = getPlatformFixtures();
   return {
-    escape: Object.values(fixtures.escape[shell]).flat(),
-    flag: Object.values(fixtures.flag[shell]).flat(),
-    quote: Object.values(fixtures.quote[shell]).flat(),
+    escape: Object.values(fixtures.escape[shellName]).flat(),
+    flag: Object.values(fixtures.flag[shellName]).flat(),
+    quote: Object.values(fixtures.quote[shellName]).flat(),
   };
 }
 

test/integration/escape-all/bash.test.js

@@ -0,0 +1,21 @@
+/**
+ * @overview Contains integration tests for `shescape.escapeAll` for the
+ * Bourne-again shell (Bash).
+ * @license MIT
+ */
+
+import test from "ava";
+
+import { constants, generate } from "../_.js";
+
+import { escapeAll } from "shescape";
+
+const runTest = constants.isWindows ? test.skip : test;
+
+runTest(`input is escaped for ${constants.binBash}`, (t) => {
+  for (const scenario of generate.escapeExamples(constants.binBash)) {
+    const { expected, input, options } = scenario;
+    const result = escapeAll([input], options);
+    t.deepEqual(result, [expected]);
+  }
+});