fix: Correctly quote NODE_OPTIONS when spaces occur in paths

This had a workaround but it turns out that it is in fact our bug.
Fixes #122

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

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;
+}

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));
+  });
+});

test/smoketest.mjs

@@ -16,17 +16,17 @@ 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"));
+runCommand("yarn", "pack", "-o", JSON.stringify(join(tmpDir, "appmap-node.tgz")));
 
 chdir(tmpDir);
 runCommand("npm", "init", "-y");
-runCommand("npm", "install", join(tmpDir, "appmap-node.tgz"));
+runCommand("npm", "install", "appmap-node.tgz");
 
 writeFileSync(
-  "index.js",
+  "index.mjs",
   `
 function main() {
   console.log("Hello world!");
@@ -35,7 +35,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");
@@ -44,7 +44,7 @@ assert(files.length === 1);
 function runCommand(command, ...args) {
   const { status } = spawnSync(command, args, {
     stdio: "inherit",
-    shell: process.platform == "win32",
+    shell: true,
   });
   assert(status === 0);
 }