Align behavior of inheriting shell with Node.js' child_process API

.github/workflows/checks.yml

@@ -179,6 +179,8 @@ jobs:
         run: npm install --global npm@8.1.2
       - name: Install dependencies
         run: npm clean-install
+      - name: Run runtime assumptions tests
+        run: npm run test:compat:assumptions
       - name: Run runtime compatibility tests
         run: npm run coverage:compat:runtime
       - name: Run experimental regexp engine compatibility tests

CONTRIBUTING.md

@@ -218,16 +218,17 @@ naming convention `_[FILENAME].js`.
 To run tests use `npm run [SCRIPT]:[MODIFIER]`, e.g. `npm run test:unit` or
 `npm run coverage:e2e`.
 
-| Script                         | Modifier         | Description                                               |
-| :----------------------------- | :--------------- | :-------------------------------------------------------- |
-| `test`, `coverage`, `mutation` | _None_           | Run tests at each level                                   |
-| `test`, `coverage`, `mutation` | `unit`           | Run unit tests                                            |
-| `test`, `coverage`, `mutation` | `integration`    | Run integration tests                                     |
-| `test`, `coverage`             | `e2e`            | Run end-to-end (e2e) tests                                |
-| `test`, `coverage`             | `compat:runtime` | Run runtime compatibility tests (current Node.js version) |
-| `test`, `coverage`             | `compat:regexp`  | Run experimental regexp engine compatibility tests        |
-| `test`, `coverage`             | `breakage`       | Run breakage tests                                        |
-| `fuzz`                         | _None_           | Run fuzz tests                                            |
+| Script                         | Modifier             | Description                                        |
+| :----------------------------- | :------------------- | :------------------------------------------------- |
+| `test`, `coverage`, `mutation` | _None_               | Run tests at each level                            |
+| `test`, `coverage`, `mutation` | `unit`               | Run unit tests                                     |
+| `test`, `coverage`, `mutation` | `integration`        | Run integration tests                              |
+| `test`, `coverage`             | `e2e`                | Run end-to-end (e2e) tests                         |
+| `test`, `coverage`             | `compat:assumptions` | Run runtime assumptions tests                      |
+| `test`, `coverage`             | `compat:runtime`     | Run runtime compatibility tests                    |
+| `test`, `coverage`             | `compat:regexp`      | Run experimental regexp engine compatibility tests |
+| `test`, `coverage`             | `breakage`           | Run breakage tests                                 |
+| `fuzz`                         | _None_               | Run fuzz tests                                     |
 
 Whenever you use the `coverage` variant of a script, a code coverage report will
 be generated at `_reports/coverage/`. Similarly, whenever you use the `mutation`

SECURITY.md

@@ -87,8 +87,8 @@ Try to include as many of the following items as possible in a security report:
 
 ### Threat Model
 
-The library considers the host system (specifically environment variables and
-the file system), as well as its configuration, to be trusted. All other inputs,
+The library considers the host system (Node environment, environment variables,
+and file system), as well as its configuration, to be trusted. All other inputs,
 most notably strings to escape, are considered untrusted. Any violation of
 confidentiality, integrity, and availability is considered a security issue.
 

package.json

@@ -143,9 +143,12 @@
     "test:integration": "npm run transpile && ava test/integration/**/*.test.js --timeout 2m",
     "test:e2e": "node script/busybox-sh.js && node script/double-link-sh.js && ava test/e2e/**/*.test.js --timeout 1m",
     "test:compat": "npm-run-all test:compat:*",
+    "test:compat:assumptions": "node test/compat/assumptions/runner.js",
+    "test:compat:assumptions:all": "nve 14.18.0,16.13.0,18.0.0,19.0.0,20.0.0,22.0.0,24.0.0,25.0.0 npm run test:compat:assumptions",
     "test:compat:runtime": "node test/compat/runtime/runner.js",
-    "test:compat:runtime:all": "nve 14.18.0,16.13.0,18.0.0,19.0.0,20.0.0,22.0.0 npm run test:compat",
+    "test:compat:runtime:all": "nve 14.18.0,16.13.0,18.0.0,19.0.0,20.0.0,22.0.0,24.0.0,25.0.0 npm run test:compat:runtime",
     "test:compat:regexp": "node --enable-experimental-regexp-engine test/compat/regexp-engine/runner.js",
+    "test:compat:regexp:all": "nve 16.13.0,18.0.0,19.0.0,20.0.0,22.0.0,24.0.0,25.0.0 npm run test:compat:regexp",
     "test:breakage": "ava test/breakage/**/*.test.js",
     "transpile": "npm-run-all transpile:*",
     "transpile:cjs": "rollup --config config/rollup.js",

src/index.js

@@ -73,7 +73,10 @@ export class Shescape {
       platform: os.platform(),
     });
 
-    options = parseOptions({ env: process.env, options }, platform);
+    options = parseOptions(
+      { env: process.env, options, version: process.version },
+      platform,
+    );
     const { flagProtection, shellName } = options;
 
     const shell = platform.getShellHelpers(shellName);

src/internal/options.js

@@ -14,6 +14,17 @@ import { hasOwn, isString } from "./reflection.js";
  */
 export const noShell = Symbol();
 
+/**
+ * Check if the Node.js version is before v22.0.0.
+ *
+ * @param {string} version The `process.version` value.
+ * @returns {boolean} `true` if version is before v22.0.0, `false` otherwise.
+ */
+function isBeforeNode22(version) {
+  const [major] = version.replace("v", "").split(".");
+  return Number.parseInt(major, 10) < 22; // eslint-disable-line no-magic-numbers
+}
+
 /**
  * Parses the `flagProtection` option.
  *
@@ -38,6 +49,7 @@ function parseFlagProtection({ options }) {
  * @param {object} args The arguments for this function.
  * @param {Object<string, string>} args.env The environment variables.
  * @param {object} args.options The options for escaping.
+ * @param {string} args.version The Node.js version.
  * @param {object} deps The dependencies for this function.
  * @param {function(): string} deps.getDefaultShell Function to get the default shell.
  * @param {function(): string} deps.getShellName Function to get the name of a shell.
@@ -46,10 +58,13 @@ function parseFlagProtection({ options }) {
  * @throws {Error} The shell is not supported or could not be found.
  */
 function parseShell(
-  { env, options },
+  { env, options, version },
   { getDefaultShell, getShellName, isShellSupported },
 ) {
-  let shell = hasOwn(options, "shell") ? options.shell : undefined;
+  let shell =
+    hasOwn(options, "shell") || isBeforeNode22(version)
+      ? options.shell
+      : undefined;
   let shellName = noShell;
 
   if (shell !== false) {
@@ -73,6 +88,7 @@ function parseShell(
  * @param {object} args The arguments for this function.
  * @param {Object<string, string>} args.env The environment variables.
  * @param {object} args.options The options for escaping.
+ * @param {string} args.version The Node.js version.
  * @param {object} deps The dependencies for this function.
  * @param {function(): string} deps.getDefaultShell Function to get the default shell.
  * @param {function(): string} deps.getShellName Function to get the name of a shell.

test/_arbitraries.js

@@ -153,9 +153,8 @@ export const process = () =>
       throwDeprecation: fc.boolean(),
       title: fc.string(),
       traceDeprecation: fc.boolean(),
-      version: fc.string(),
       versions: fc.record({
-        node: fc.string(),
+        node: semver(),
         v8: fc.string(),
         uv: fc.string(),
         zlib: fc.string(),
@@ -174,9 +173,34 @@ export const process = () =>
     })
     .map((process) => {
       process.argv0 = process.argv[0];
+      process.version = `v${process.versions.node}`;
       return process;
     });
 
+/**
+ * The semver arbitrary generates semver version strings.
+ *
+ * @param {object} [args] Configuration for the arbitrary.
+ * @param {number} [args.maxMajor] The maximum major version.
+ * @param {number} [args.minMajor] The minimum major version.
+ * @returns {string} Arbitrary semver version strings.
+ */
+export const semver = ({ maxMajor, minMajor } = {}) => {
+  minMajor ||= 0;
+  return fc
+    .tuple(
+      fc.oneof(
+        fc.constantFrom(
+          ...[minMajor, maxMajor].filter((value) => value !== undefined),
+        ),
+        fc.integer({ min: minMajor, max: maxMajor }),
+      ),
+      fc.integer({ min: 0 }),
+      fc.integer({ min: 0 }),
+    )
+    .map(([major, minor, patch]) => `${major}.${minor}.${patch}`);
+};
+
 /**
  * The shescapeArg arbitrary generates strings that could be inputs to the
  * Shescape API for escaping.

test/compat/assumptions/child-process.test.js

@@ -0,0 +1,46 @@
+/**
+ * @overview Contains tests for assumptions this library makes about the Node.js
+ * builtin module `node:child_process`.
+ * @license MIT
+ */
+
+import * as cp from "node:child_process";
+import * as process from "node:process";
+
+const nodeMajorVersion = process.versions.node.split(".")[0];
+
+/**
+ * Test if the 'shell' value from the options prototype is used.
+ *
+ * @throws {Error} If the test fails.
+ */
+export function testShellInheritance() {
+  const command = "echo 'Hello world!'";
+  const options = {
+    shell: "this is definitely not a real shell",
+  };
+
+  let errOwn = false;
+  try {
+    cp.execSync(command, options);
+  } catch {
+    errOwn = true;
+  }
+
+  let errProto = false;
+  try {
+    Object.prototype.shell = options.shell; // eslint-disable-line no-extend-native
+    cp.execSync(command, {});
+  } catch {
+    errProto = true;
+  } finally {
+    delete Object.prototype.shell;
+  }
+
+  if (
+    (nodeMajorVersion >= 22 && errOwn === errProto) ||
+    (nodeMajorVersion < 22 && errOwn !== errProto)
+  ) {
+    throw new Error(`own shell error ${errOwn}, proto shell error ${errProto}`);
+  }
+}

test/compat/assumptions/runner.js

@@ -0,0 +1,19 @@
+/* eslint-disable ava/no-import-test-files */
+
+/**
+ * @overview The test runner for assumption compatibility tests.
+ * @license MIT
+ *
+ * This test suite intentionally doesn't use a test framework since the tests
+ * are required to run on old Node.js versions - which may not be supported by
+ * the test framework used - and the tests are relatively simple anyway.
+ * Instead, this minimal test runner is used, where execution just terminates
+ * early with a non-zero exit code when a test fails.
+ *
+ * This may be migrated to the Node.js' builtin testing library when that is
+ * available on the lowest Node.js version supported by the library.
+ */
+
+import * as cp from "./child-process.test.js";
+
+cp.testShellInheritance();

test/unit/options/parse-options.test.js

@@ -15,10 +15,11 @@ import { arbitrary } from "./_.js";
 
 const arbitraryInput = () =>
   fc
-    .tuple(arbitrary.shescapeOptions(), arbitrary.env())
-    .map(([options, env]) => {
+    .tuple(arbitrary.env(), arbitrary.shescapeOptions(), arbitrary.semver())
+    .map(([env, options, version]) => {
       options ||= {};
-      return { env, options };
+      version = `v${version}`;
+      return { env, options, version };
     });
 
 test.beforeEach((t) => {
@@ -171,6 +172,51 @@ testProp(
   },
 );
 
+testProp(
+  "shell is inherited (before Node.js v22)",
+  [arbitraryInput(), fc.string(), arbitrary.semver({ maxMajor: 21 })],
+  (t, args, inheritedShell, version) => {
+    t.context.deps.getShellName.resetHistory();
+
+    delete args.options.shell;
+    args.version = `v${version}`;
+
+    args.options = Object.assign(
+      Object.create({ shell: inheritedShell }),
+      args.options,
+    );
+
+    parseOptions(args, t.context.deps);
+    t.is(t.context.deps.getShellName.callCount, 1);
+    t.true(
+      t.context.deps.getShellName.calledWith(
+        sinon.match({ shell: inheritedShell }),
+      ),
+    );
+  },
+);
+testProp(
+  "shell is not inherited (Node.js v22 or later)",
+  [arbitraryInput(), fc.string(), arbitrary.semver({ minMajor: 22 })],
+  (t, args, inheritedShell, version) => {
+    t.context.deps.getShellName.resetHistory();
+
+    delete args.options.shell;
+    args.version = `v${version}`;
+
+    args.options = Object.assign(
+      Object.create({ shell: inheritedShell }),
+      args.options,
+    );
+
+    parseOptions(args, t.context.deps);
+    t.is(t.context.deps.getShellName.callCount, 1);
+    t.true(
+      t.context.deps.getShellName.calledWith(sinon.match({ shell: undefined })),
+    );
+  },
+);
+
 testProp("shell is supported", [arbitraryInput()], (t, args) => {
   t.context.deps.isShellSupported.resetHistory();
   t.context.deps.isShellSupported.returns(true);