From 841646fc7b195f83b93aa34efd030b811cb81754 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= <divided.mind@gmail.com>
Date: Mon, 18 Mar 2024 17:46:28 +0100
Subject: [PATCH] fix: Correctly quote NODE_OPTIONS when spaces occur in paths

Fixes #122
---
 src/bin.ts                             |  5 +--
 src/util/NodeOptions.ts                | 49 ++++++++++++++++++++++++++
 src/util/__tests__/NodeOptions.test.ts | 15 ++++++++
 test/smoketest.mjs                     |  7 ++--
 4 files changed, 71 insertions(+), 5 deletions(-)
 create mode 100644 src/util/NodeOptions.ts
 create mode 100644 src/util/__tests__/NodeOptions.test.ts

diff --git a/src/bin.ts b/src/bin.ts
index 8cc4be60..79650c5f 100644
--- a/src/bin.ts
+++ b/src/bin.ts
@@ -11,6 +11,7 @@ import YAML from "yaml";
 import config from "./config";
 import { info } from "./message";
 import { version } from "./metadata";
+import NodeOptions from "./util/NodeOptions";
 import forwardSignals from "./util/forwardSignals";
 import { readPkgUp } from "./util/readPkgUp";
 
@@ -101,9 +102,9 @@ function isTsEsmLoaderNeeded(cmd: string, args: string[]) {
 }
 
 function addNodeOptions(...options: string[]) {
-  const envar = (process.env.NODE_OPTIONS ?? "").split(" ");
+  const envar = new NodeOptions(process.env.NODE_OPTIONS);
   envar.push(...options);
-  process.env.NODE_OPTIONS = envar.join(" ");
+  process.env.NODE_OPTIONS = envar.toString();
 }
 
 /* Heuristic to check if argument is a node script. Currently just checks if it's an existing file
diff --git a/src/util/NodeOptions.ts b/src/util/NodeOptions.ts
new file mode 100644
index 00000000..5e934cfa
--- /dev/null
+++ b/src/util/NodeOptions.ts
@@ -0,0 +1,49 @@
+export default class NodeOptions extends Array {
+  constructor(value?: string) {
+    super();
+    if (!value) return;
+    if (typeof value !== "string") throw new Error("NODE_OPTIONS must be a string");
+
+    this.push(...parseNodeOptionsEnvVar(value));
+  }
+
+  toString(): string {
+    return this.map(maybeQuote).join(" ");
+  }
+
+  static get [Symbol.species]() {
+    return Array;
+  }
+}
+
+// based on ParseNodeOptionsEnvVar()
+// https://github.com/nodejs/node/blob/39f1b899cd536de4d4c9bbf56f24927d8d06999a/src/node_options.cc#L1397
+function parseNodeOptionsEnvVar(nodeOptions: string): string[] {
+  const result: string[] = [];
+  let buffer = "";
+  let inString = false;
+  for (let i = 0; i < nodeOptions.length; i++) {
+    let c = nodeOptions[i];
+    if (c === "\\" && inString) {
+      if (i + 1 === nodeOptions.length) break;
+      c = nodeOptions[++i];
+    } else if (c === " " && !inString) {
+      result.push(buffer);
+      buffer = "";
+      continue;
+    } else if (c === '"') {
+      inString = !inString;
+      continue;
+    }
+
+    buffer += c;
+  }
+
+  result.push(buffer);
+  return result;
+}
+
+function maybeQuote(value: string): string {
+  if (value.includes(" ")) return `"${value.replace("\\", "\\\\").replace('"', '\\"')}"`;
+  return value;
+}
diff --git a/src/util/__tests__/NodeOptions.test.ts b/src/util/__tests__/NodeOptions.test.ts
new file mode 100644
index 00000000..f52750b2
--- /dev/null
+++ b/src/util/__tests__/NodeOptions.test.ts
@@ -0,0 +1,15 @@
+import NodeOptions from "../NodeOptions";
+
+describe(NodeOptions, () => {
+  it("splits the options correctly", () => {
+    const options = new NodeOptions('--option "spaced argument"');
+    expect(Array.from(options)).toEqual(["--option", "spaced argument"]);
+  });
+
+  it("stringifies spaced options properly", () => {
+    const opts = new NodeOptions("");
+    opts.push("--option", 'spaced \\argu"ment');
+    expect(opts.toString()).toBe('--option "spaced \\\\argu\\"ment"');
+    expect(Array.from(new NodeOptions(opts.toString()))).toEqual(Array.from(opts));
+  });
+});
diff --git a/test/smoketest.mjs b/test/smoketest.mjs
index 28d225e8..a01b8c33 100755
--- a/test/smoketest.mjs
+++ b/test/smoketest.mjs
@@ -16,7 +16,8 @@ import tmp from "tmp";
  * it doesn't crash and produces an AppMap. */
 
 // Create a temporary directory to work in.
-const tmpDir = tmp.dirSync({ unsafeCleanup: true }).name;
+// To test NODE_OPTIONS handling, use a space in the directory name and use mjs.
+const tmpDir = tmp.dirSync({ unsafeCleanup: true, template: "appmap node test XXXXXX" }).name;
 
 // Package up the appmap-node package.
 runCommand("yarn", "pack", "-o", join(tmpDir, "appmap-node.tgz"));
@@ -26,7 +27,7 @@ runCommand("npm", "init", "-y");
 runCommand("npm", "install", join(tmpDir, "appmap-node.tgz"));
 
 writeFileSync(
-  "index.js",
+  "index.mjs",
   `
 function main() {
   console.log("Hello world!");
@@ -35,7 +36,7 @@ main();
 `,
 );
 
-runCommand("npm", "exec", "appmap-node", "index.js");
+runCommand("npm", "exec", "appmap-node", "index.mjs");
 
 // verify that appmap has been created
 const files = glob.globSync("tmp/**/*.appmap.json");