Tests for non-own property access (#1280)

Add a test to check for non-own property access which could be
manipulated by prototype pollution. Fix such accesses to check
if it is an own property first.

Because this is standard Node.js behavior this is considered a
hardening measure (rather than a security fix).

CHANGELOG.md

@@ -7,7 +7,7 @@ Versioning].
 
 ## [Unreleased]
 
-- _No changes yet_
+- Harden against polluted prototypes. ([#1280])
 
 ## [2.0.1] - 2023-10-28
 
@@ -318,6 +318,7 @@ Versioning].
 [#1137]: https://github.com/ericcornelissen/shescape/pull/1137
 [#1142]: https://github.com/ericcornelissen/shescape/pull/1142
 [#1149]: https://github.com/ericcornelissen/shescape/pull/1149
+[#1280]: https://github.com/ericcornelissen/shescape/pull/1280
 [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

script/run-platform-coverage.js

@@ -15,4 +15,5 @@ if (!testType) {
   process.exit(1);
 }
 
-common.npm(["run", `coverage:${testType}:${platform}`]);
+const cmd = common.npm(["run", `coverage:${testType}:${platform}`]);
+cmd.on("close", (code) => process.exit(code));

src/options.js

@@ -24,14 +24,26 @@ function unsupportedError(shellName) {
   return `Shescape does not support the shell ${shellName}`;
 }
 
+/**
+ * Check if the given object has the given property as an own property.
+ *
+ * This custom function is used over `Object.hasOwn` because that isn't
+ * available in all supported Node.js versions.
+ *
+ * @param {object} object The object of interest.
+ * @param {string} property The property of interest.
+ * @returns {boolean} `true` if property is an own-property, `false` otherwise.
+ */
+function hasOwn(object, property) {
+  return Object.prototype.hasOwnProperty.call(object, property);
+}
+
 /**
  * Parses options provided to shescape.
  *
  * @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 {boolean} [args.options.flagProtection] Is flag protection enabled.
- * @param {boolean | string} [args.options.shell=true] The shell to escape for.
  * @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.
@@ -40,9 +52,14 @@ function unsupportedError(shellName) {
  * @throws {Error} The shell is not supported or could not be found.
  */
 export function parseOptions(
-  { env, options: { flagProtection, shell } },
+  { env, options },
   { getDefaultShell, getShellName, isShellSupported },
 ) {
+  let flagProtection = hasOwn(options, "flagProtection")
+    ? options.flagProtection
+    : undefined;
+  let shell = hasOwn(options, "shell") ? options.shell : undefined;
+
   flagProtection =
     flagProtection === undefined ? true : flagProtection ? true : false;
 

test/integration/constructor/_.js

@@ -0,0 +1,9 @@
+/**
+ * @overview Provides testing utilities.
+ * @license MIT
+ */
+
+import * as arbitrary from "../../_arbitraries.js";
+import * as pollution from "./_pollution.js";
+
+export { arbitrary, pollution };

test/integration/constructor/_pollution.js

@@ -0,0 +1,98 @@
+/**
+ * @overview Contains helpers to detect non-own property access.
+ * @license MIT
+ */
+
+import assert from "node:assert/strict";
+
+/**
+ * A store for non-own property accesses by proxy.
+ */
+const pollutionState = new Map();
+
+/**
+ * Check whether or not a value is nil (`null` or `undefined`).
+ *
+ * @param {any} value The value to check.
+ * @returns {boolean} `true` if `value` is nil, `false` otherwise.
+ */
+function isNil(value) {
+  return value === null || value === undefined;
+}
+
+/**
+ * Check whether or not a value is a primitive value.
+ *
+ * @param {any} value The value to check.
+ * @returns {boolean} `true` if `value` is primitive, `false` otherwise.
+ */
+function isPrimitive(value) {
+  return typeof value === "number" || typeof value === "string";
+}
+
+/**
+ * Check whether or not a value can be wrapped by a `Proxy`.
+ *
+ * @param {any} value The value to check.
+ * @returns {boolean} `true` if `value` is proxyable, `false` otherwise.
+ */
+function isProxyable(value) {
+  return !(isNil(value) || isPrimitive(value));
+}
+
+/**
+ * Wrap the provided target in in order to monitor it for access to properties
+ * not present on the target.
+ *
+ * @param {any} target The value to wrap.
+ * @returns {any} The wrapped value (or original if wrapping isn't possible).
+ */
+export function wrap(target) {
+  if (!isProxyable(target)) {
+    return target;
+  }
+
+  const nonOwnAccesses = new Set();
+  const proxy = new Proxy(target, {
+    get(target, property, _proxy) {
+      if (!Object.hasOwn(target, property)) {
+        nonOwnAccesses.add(property);
+      }
+
+      // Return normal lookup to ensure normal test execution.
+      return target[property];
+    },
+  });
+
+  pollutionState.set(proxy, nonOwnAccesses);
+  return proxy;
+}
+
+/**
+ * Check if non-own property access was detected on the given wrapped object.
+ *
+ * @param {any} wrapped A `wrap`ped value.
+ * @throws {Error} If non-own property access was detected.
+ */
+export function check(wrapped) {
+  if (!isProxyable(wrapped)) {
+    return;
+  }
+
+  assert.ok(pollutionState.has(wrapped), "target not found");
+  const nonOwnAccesses = pollutionState.get(wrapped);
+
+  // Remove the proxy from the state so re-use of it does not result in errors
+  // from one test to affect other tests. (Also just to reduce memory usage.)
+  pollutionState.delete(wrapped);
+
+  const actual = nonOwnAccesses.size;
+  const expected = 0;
+  const propertiesList = Array.from(nonOwnAccesses.values()).join(", ");
+
+  assert.equal(
+    actual,
+    expected,
+    `Non-own access to ${actual} property(s) detected: ${propertiesList}`,
+  );
+}

test/integration/constructor/constructor.test.js

@@ -3,12 +3,28 @@
  * @license MIT
  */
 
+import { testProp } from "@fast-check/ava";
 import test from "ava";
 
+import { arbitrary, pollution } from "./_.js";
+
 import { Shescape } from "shescape";
 
 test("shell is unsupported", (t) => {
   const shell = "not-actually-a-shell-that-exists";
 
   t.throws(() => new Shescape({ shell }), { instanceOf: Error });
 });
+
+testProp(
+  "affected by prototype pollution",
+  [arbitrary.shescapeOptions().map(pollution.wrap)],
+  (t, options) => {
+    try {
+      new Shescape(options);
+    } catch (_) {}
+
+    pollution.check(options);
+    t.pass();
+  },
+);